strapi / strapi

🚀 Strapi is the leading open-source headless CMS. It’s 100% JavaScript/TypeScript, fully customizable, and developer-first.
https://strapi.io
Other
63.84k stars 8.13k forks source link

Database connection using `connection` URL #3094

Closed genu closed 4 years ago

genu commented 5 years ago

Informations

What is the current behavior? When deploying to heroku, my database config is set as:

{
  "defaultConnection": "default",
  "connections": {
    "default": {
      "connector": "strapi-hook-bookshelf",
      "settings": {
        "client": "pg",
        "connection": "${process.env.DATABASE_URL}",
        "ssl": "${process.env.DATABASE_SSL || false}"
      },
      "options": {}
    }
  }
}

When strapi starts up, it errors out with ECONNREFUSED 127.0.0.1:5432.

Steps to reproduce the problem Set up a database connection using the URI only.

What is the expected behavior? The expectation is that a database connection can be established using URL only.

Heroku Postgres, for example, rotates database credentials and updates the DATABASE_URL automatically, therefore, strapi won't pick up the new credentials unless the connection is pulled from the DATABASE_URL environment variable.

Suggested solutions In the knex and bookshelf hooks I believe there are some defaults for setting host to localhost. I'm not sure why that is needed.

davidkartuzinski commented 5 years ago

@genu Hello! You can take a look at the new documentation about deploying Strapi to Heroku. https://strapi.io/documentation/3.x.x/guides/deployment.html#heroku - There is a section on the config. The PR for these docs got merged yesterday.

At the moment, we have to split the URL into its key/value pairs as covered in the docs.

Please let me know if you have any questions.

lauriejim commented 5 years ago

Here is the way we connect to the database and the thing you are trying to do is impossible. https://github.com/strapi/strapi/blob/master/packages/strapi-hook-knex/lib/index.js#L87 connection key doesn't exist. Please follow the documentation for this point.

genu commented 5 years ago

@davidkartuzinski

Thanks for the documentation link.

The information about the database setup, however, is wrong. It should be updated.

Splitting up the connection string as the docs say does not work when using Heroku Postgres, because Heroku rotates credentials periodically. When the credentials rotate, heroku updates the connection string in the environment settings. It DOES NOT, however, update the extra DB variables that were created after the fact.

Users would have to update the env variables every time the credentials are rotated (once a week?) --- Its clearly not ideal.

Perhaps, that section in the docs should just remove the Heroku postgres setup, since that is the only option that would have the issue.

@lauriejim I looked over the code, I see your point. I think there is another issue relating to adopting all config options from knex, in the hooks package -- I think that is the ideal solution since knex does have a connection string option.

genu commented 5 years ago

@lauriejim

A workaround, for now, would be to add a helper function inside the hooks package that splits up a connection string if it exists into the required parts.

That is what knex does.

lauriejim commented 5 years ago

Okay thank you for this details.

vitaly-t commented 5 years ago

A workaround, for now, would be to add a helper function inside the hooks package that splits up a connection string if it exists into the required parts.

Generic connection-string does it well.

alexandrebodin commented 5 years ago

Hi guys,

Several things to say here:

We are indeed missing support for a connection string for the bookshelf hook. We are going to be working a lot on the database architecture so I wouldn't recommend you guys spending much time on a PR that won't be merged anytime soon.

I however suggest you guys to simple your database.json to database.js for now to handle this usecase until we support it natively :)

const parse = require('connection-string');

let settings = {
  client: 'sqlite',
  host: `${process.env.DATABASE_HOST || '127.0.0.1'}`,
  port: `${process.env.DATABASE_PORT || 27017}`,
  database: `${process.env.DATABASE_NAME || 'strapi'}`,
  username: `${process.env.DATABASE_USERNAME || ''}`,
  password: `${process.env.DATABASE_PASSWORD || ''}`,
};

if (process.env.DATABASE_URL) {
  const parsed = parse(process.env.DATABASE_URL);
  // parse your stuff
  // read examples here https://github.com/vitaly-t/connection-string/wiki/Adapters
  settings = convertToMyClient(parsed);
}

module.exports = {
  defaultConnection: 'default',
  connections: {
    connector: 'strapi-hook-mongoose',
    settings,
    options: {},
  },
};

Hope this helped

websocket98765 commented 5 years ago

@alexandrebodin Is simply renaming database.json to database.js enough to get this file loaded? Or where is the reference that load the file that needs to be changed also? Thanks in advance.

vitaly-t commented 5 years ago

Re-posting updated @alexandrebodin solution, since the latest/TypeScript connection-string (after v3.0.0.) no longer supports legacy function style, and only classes now:

const {ConnectionString} = require('connection-string');

let settings = {
  client: 'sqlite',
  host: `${process.env.DATABASE_HOST || '127.0.0.1'}`,
  port: `${process.env.DATABASE_PORT || 27017}`,
  database: `${process.env.DATABASE_NAME || 'strapi'}`,
  username: `${process.env.DATABASE_USERNAME || ''}`,
  password: `${process.env.DATABASE_PASSWORD || ''}`,
};

if (process.env.DATABASE_URL) {
  const parsed = new ConnectionString(process.env.DATABASE_URL);
  // parse your stuff
  // read examples here https://github.com/vitaly-t/connection-string/wiki/Adapters
  settings = convertToMyClient(parsed);
}

module.exports = {
  defaultConnection: 'default',
  connections: {
    default: {
      connector: 'strapi-hook-mongoose',
      settings,
      options: {}
    }
  }
};
websocket98765 commented 5 years ago

Both the code examples in this thread have errors that make them not work.

@vitaly-t 1. On module.exports, the connections property needs to contain a value that is an object whose keys are the name of each connection (e.g. default). 2. The connection-string package has an error where it will only set ssl to true or undefined. I have't looked into that package's repo, but off hand it appears to be because, after parsing, the cs object represents booleans as a string params: { ssl: 'false' } and any string given to Boolean() evaluates to true. It would be better if your parsed value were already a boolean, such as params: { ssl: false }.

I've simplified and made a fully working PostgreSQL example.

UPDATE: Code removed because I posted an improved version below that doesn't require any third party dependency for parsing: https://github.com/strapi/strapi/issues/3094#issuecomment-538784881

vitaly-t commented 5 years ago

@websocket98765 Library connection-string does not parse values specified within the parameters, because it is not supposed to, as it has no knowledge of what is in there.

Even a boolean can be defined as true, false, yes, no, 1, 0, etc...

The onus is on your to do the conversion, as only the client knows what values it supports.

So replace that line with something like:

settings.ssl = cs.params && (cs.params.ssl === 'true' || cs.params.ssl === '1');

And besides, ssl can have a number of variations in which it is supported. For example, see this PR where multiple SSL-certificate details are supported via connection string.

connection-string is completely generic, which means you can define the parameter protocol as you like.

websocket98765 commented 5 years ago

Updated version that removes dependency of connection-string. (No offense intended to that library, just given the sensitivity of these credentials, it's preferable to use the built-in url module to parse them when possible, instead of any NPM package.) Works for PostgreSQL, untested for other databases.

/**
 * `config/environments/production/database.js`
 *
 * database.js parses a PostgreSQL DB connection URL into the parts needed
 * by Strapi. This replaces Strapi's default database.json. It's cleaner and
 * less error prone because it needs only 1 env var (DATABASE_URL) instead of 6.
 * DATABASE_URL should be a valid URL such as:
 * postgres://username:password@hostname:port/dbname?ssl=false
 */

const url = require('url');

let settings = {
  client: 'postgres'
};

// For some reason, Strapi loads this file even in non-production environments.
// The conditional prevents Strapi from failing to start due to
// process.env.DATABASE_URL being undefined in other, non-production environments.
if (process.env.DATABASE_URL) {
  const parsed = url.parse(process.env.DATABASE_URL, true);
  const [username, password] = parsed.auth.split(':');

  settings.host     = parsed.hostname;
  settings.port     = Number(parsed.port);
  settings.database = parsed.pathname.substr(1);
  settings.username = username;
  settings.password = password;
  settings.ssl      = (parsed.query.ssl === 'true');
}

module.exports = {
  defaultConnection: 'default',
  connections: {
    default: {
      connector: 'strapi-hook-bookshelf',
      settings,
      options: {}
    }
  }
};
vitaly-t commented 5 years ago

@websocket98765

given the sensitivity of these credentials, it's preferable to use the built-in url module to parse them when possible, instead of any NPM package

No offence, but your argument makes no sense. And the default URL parser will fail parsing a valid connection string in more cases than I would be willing to list, hence the reason connection-string was created in the first place.

websocket98765 commented 5 years ago

Dude, really, I intended no offense. My code is posted for anyone who would prefer to use built-in modules, instead of a third-party library because it is unarguably better to use built-in library--when it gets the job done as it did for mine--rather than a third party library. If my code doesn't work for other use cases, as you say, people can consider your library too. They are now two choices.

(As far as why it'd better to remove dependencies when not needed, that should be obvious, but for less obvious security-related reasons that you might find interesting: https://www.csoonline.com/article/3214624/malicious-code-in-the-node-js-npm-registry-shakes-open-source-trust-model.html. And I'd certainly give you the benefit of the doubt of being trustworthy, but good security practice says to remove need for trust when it's not needed.).

But in any case, you might consider updating your code example in this thread so it works. It'd be respectful to future googlers. I know that I spent a few hours troubleshooting why your code example wasn't working in Strapi before discovering the module.exports is incorrectly formed, as I noted in my first comment to you. And also the SSL line in your wiki for postgres needs to be changed from Boolean(cs.params.ssl) to something else, because if you're going to keep cs.params.ssl as a string, passing a string ('false' or 'true') to Boolean() will always return true. Cheers man.

vitaly-t commented 5 years ago

@websocket98765 I don't understand which part of my code example doesn't work. Where is the problem there?

Note that I merely copied the example from @alexandrebodin , and changed how it imports the library's class ConnectionString, nothing else. If something else doesn't work in that example, then it is not even my example to correct, as I wouldn't know.

websocket98765 commented 5 years ago

I don't understand which part of my code example doesn't work. Where is the problem there?

It's the part I mentioned earlier:

// FAILS
module.exports = {
  defaultConnection: 'default',
  connections: {
    connector: 'strapi-hook-mongoose',
    settings,
    options: {},
  },
};

needs to be:

module.exports = {
  defaultConnection: 'default',
  connections: {
    default: {
      connector: 'strapi-hook-mongoose',
      settings,
      options: {}
    }
  }
};

or else Strapi fails.

Note that I merely copied the example from @alexandrebodin , and changed how it imports the library's class ConnectionString, nothing else. If something else doesn't work in that example, then it is not even my example to correct, as I wouldn't know.

I realize the error didn't originate with you. Neither of you tested your code :) It'd be nice if you both had noted it was untested. But for now, could at least update your version of code to be working, since only you can edit your post, so hopefully others don't waste time on the same error. (And hopefully Alexandre does the same if he reads this thread.) Although at this point, it probably doesn't matter as much since future googlers will see our recent posts about the error and can find at least one functional version in this thread.

vitaly-t commented 5 years ago

Ok, I have updated my quoted example.

websocket98765 commented 5 years ago

Ok, I have updated my quoted example.

Thanks

serman commented 5 years ago

Hi! I found this patch because heroku is causing me a headache changing the POSTGRES database user/name ... from time to time

I removed (actually renamed to database.json.bak) the config/environments/production/database.json and created the database.js following the instructions from @websocket98765 ( https://github.com/strapi/strapi/issues/3094#issuecomment-538784881 ) but I get this error when strapi starts

> node server.js
2019-10-25T01:30:48.020450+00:00 app[web.1]:
2019-10-25T01:30:51.060454+00:00 app[web.1]: [2019-10-25T01:30:51.059Z] warn (hook:bookshelf) wasn't loaded due to missing key `enabled` in the configuration
2019-10-25T01:30:51.062283+00:00 app[web.1]: [2019-10-25T01:30:51.062Z] warn (hook:knex) wasn't loaded due to missing key `enabled` in the configuration
2019-10-25T01:30:51.342682+00:00 app[web.1]: error TypeError: strapi.models.core_store.forge is not a function
2019-10-25T01:30:51.342689+00:00 app[web.1]: at Object.get (/app/node_modules/strapi/lib/core/store.js:55:47)
2019-10-25T01:30:51.342691+00:00 app[web.1]: at module.exports (/app/plugins/content-manager/config/functions/bootstrap.js:312:42)
2019-10-25T01:30:51.342696+00:00 app[web.1]: at /app/node_modules/strapi/lib/Strapi.js:299:13
2019-10-25T01:30:51.342698+00:00 app[web.1]: at new Promise (<anonymous>)
2019-10-25T01:30:51.342700+00:00 app[web.1]: at execBootstrap (/app/node_modules/strapi/lib/Strapi.js:287:11)
2019-10-25T01:30:51.342702+00:00 app[web.1]: at /app/node_modules/strapi/lib/Strapi.js:326:44
2019-10-25T01:30:51.342704+00:00 app[web.1]: at Array.map (<anonymous>)
2019-10-25T01:30:51.342705+00:00 app[web.1]: at Strapi.bootstrap (/app/node_modules/strapi/lib/Strapi.js:326:35)
2019-10-25T01:30:51.342707+00:00 app[web.1]: at Strapi.start (/app/node_modules/strapi/lib/Strapi.js:109:18)
2019-10-25T01:30:51.345282+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: TypeError: strapi.models.core_store.forge is not a function
2019-10-25T01:30:51.345285+00:00 app[web.1]: at Object.get (/app/node_modules/strapi/lib/core/store.js:55:47)
2019-10-25T01:30:51.345296+00:00 app[web.1]: at module.exports (/app/plugins/settings-manager/config/functions/bootstrap.js:17:26)
2019-10-25T01:30:51.345298+00:00 app[web.1]: at /app/node_modules/strapi/lib/Strapi.js:299:13
2019-10-25T01:30:51.345299+00:00 app[web.1]: at new Promise (<anonymous>)
2019-10-25T01:30:51.345301+00:00 app[web.1]: at execBootstrap (/app/node_modules/strapi/lib/Strapi.js:287:11)
2019-10-25T01:30:51.345302+00:00 app[web.1]: at /app/node_modules/strapi/lib/Strapi.js:326:44
2019-10-25T01:30:51.345304+00:00 app[web.1]: at Array.map (<anonymous>)
2019-10-25T01:30:51.345305+00:00 app[web.1]: at Strapi.bootstrap (/app/node_modules/strapi/lib/Strapi.js:326:35)
2019-10-25T01:30:51.345307+00:00 app[web.1]: at Strapi.start (/app/node_modules/strapi/lib/Strapi.js:109:18)
2019-10-25T01:30:51.345525+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 4)
2019-10-25T01:30:51.345687+00:00 app[web.1]: (node:23) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
2019-10-25T01:30:51.345897+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: TypeError: strapi.models.core_store.forge is not a function
2019-10-25T01:30:51.345900+00:00 app[web.1]: at Object.get (/app/node_modules/strapi/lib/core/store.js:55:47)
2019-10-25T01:30:51.345901+00:00 app[web.1]: at module.exports (/app/plugins/users-permissions/config/functions/bootstrap.js:96:45)
2019-10-25T01:30:51.345902+00:00 app[web.1]: at /app/node_modules/strapi/lib/Strapi.js:299:13
2019-10-25T01:30:51.345904+00:00 app[web.1]: at new Promise (<anonymous>)
2019-10-25T01:30:51.345905+00:00 app[web.1]: at execBootstrap (/app/node_modules/strapi/lib/Strapi.js:287:11)
2019-10-25T01:30:51.345906+00:00 app[web.1]: at /app/node_modules/strapi/lib/Strapi.js:326:44
2019-10-25T01:30:51.345908+00:00 app[web.1]: at Array.map (<anonymous>)
2019-10-25T01:30:51.345909+00:00 app[web.1]: at Strapi.bootstrap (/app/node_modules/strapi/lib/Strapi.js:326:35)
2019-10-25T01:30:51.345910+00:00 app[web.1]: at Strapi.start (/app/node_modules/strapi/lib/Strapi.js:109:18)
2019-10-25T01:30:51.346016+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 5)
2019-10-25T01:30:51.370863+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: ReferenceError: config is not defined
2019-10-25T01:30:51.370867+00:00 app[web.1]: at /app/plugins/email/config/functions/bootstrap.js:68:40
2019-10-25T01:30:51.371019+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 7)
2019-10-25T01:30:51.376732+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: ReferenceError: config is not defined
2019-10-25T01:30:51.376735+00:00 app[web.1]: at /app/plugins/upload/config/functions/bootstrap.js:70:40
2019-10-25T01:30:51.376856+00:00 app[web.1]: (node:23) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 9)

Any tip?

alexandrebodin commented 5 years ago

@serman You have set your connector to be strapi-hook-mongoose when copying the example but yours should be strapi-hook-bookshelf

serman commented 5 years ago

Thanks for your reply! But I don't think that is the problem, since in the example that I am copying ( https://github.com/strapi/strapi/issues/3094#issuecomment-538784881 ) the connector is:

connector: 'strapi-hook-bookshelf',
alexandrebodin commented 5 years ago

@serman can you share your file ? just to check you aren't missing sth

serman commented 5 years ago

I removed the database.json and created the database.js


/**
 * `config/environments/production/database.js`
 *
 * database.js parses a PostgreSQL DB connection URL into the parts needed
 * by Strapi. This replaces Strapi's default database.json. It's cleaner and
 * less error prone because it needs only 1 env var (DATABASE_URL) instead of 6.
 * DATABASE_URL should be a valid URL such as:
 * postgres://username:password@hostname:port/dbname?ssl=false
 */

const url = require('url');

let settings = {
  client: 'postgres'
};

// For some reason, Strapi loads this file even in non-production environments.
// The conditional prevents Strapi from failing to start due to
// process.env.DATABASE_URL being undefined in other, non-production environments.
if (process.env.DATABASE_URL) {
  const parsed = url.parse(process.env.DATABASE_URL, true);
  const [username, password] = parsed.auth.split(':');

  settings.host     = parsed.hostname;
  settings.port     = Number(parsed.port);
  settings.database = parsed.pathname.substr(1);
  settings.username = username;
  settings.password = password;
  settings.ssl      = (parsed.query.ssl === 'true');
}

module.exports = {
  defaultConnection: 'default',
  connections: {
    default: {
      connector: 'strapi-hook-bookshelf',
      settings,
      options: {}
    }
  }
}; 

Do I need to update strapi to the latest version for this to work? I am testing this on a Strapi v3.0.0-alpha.26.2

alexandrebodin commented 5 years ago

Ho yep you should use strapi beta now. Alpha is not maintained at all

serman commented 5 years ago

ok, thanks!

jonathanstanley commented 4 years ago

Spent a couple hours until I found this thread. Glad I found this before changing https://github.com/strapi/strapi/blob/8d0c6848ba421680570e974e5d8606314735de3e/packages/strapi-connector-bookshelf/lib/knex.js#L93-L106 Kept getting error Impossible to register the 'logcategory.log' model. and error Error: connect ECONNREFUSED 127.0.0.1: and SSL off Thanks for the solutions all! I got postgres working on heroku with a slight modification from @websocket98765 https://github.com/strapi/strapi/issues/3094#issuecomment-538784881 (changed below to set SSL=true).

/**
 * `config/environments/production/database.js`
 *
 * replace database.json with database.js 
 * parses a PostgreSQL DB connection URL into the parts needed
 * by Strapi. Without this, heroku may throw ECONNREFUSED 127.0.0.1:xxxx.  
 */

const url = require('url');

let settings = {
  client: 'postgres'
};

if (process.env.DATABASE_URL) {
  const parsed = url.parse(process.env.DATABASE_URL, true);
  const [username, password] = parsed.auth.split(':');

  settings.host     = parsed.hostname;
  settings.port     = Number(parsed.port);
  settings.database = parsed.pathname.substr(1);
  settings.username = username;
  settings.password = password;
  settings.ssl      = true;
}

module.exports = {
  defaultConnection: 'default',
  connections: {
    default: {
      connector: 'bookshelf',
      settings,
      options: {}
    }
  }
};
lauriejim commented 4 years ago

Since it will be part of the database restructure in coming (long but in coming) based on the Alex comment I close this issue. No longer need to let this issue open. Thank you for your contribution on this discussion.