Repository: CSCfi/beacon-python

Issue:
Special characters in database password
#### Describe the bug
If your db password contains certain special characters , the call to `asyncpg.create_pool` will fail, as some characters are interpreted as a url delimiters.

#### To Reproduce
Steps to reproduce the behaviour:
1. Have a password containing special characters.
2. Run the beacon.
3. Make a call to the beacon.
4. Get `.../.local/lib/python3.6/site-packages/asyncpg/connect_utils.py", line 187, in _parse_hostlist
    hostlist_ports.append(int(hostspec_port))`


#### Possible solution
In `beacon_api/conf/config.py`:
```py
async def init_db_pool():

    return await asyncpg.create_pool(host=POSTGRES['host'],   # eg "localhost"
                                     port=POSTGRES['port'],
                                     user=POSTGRES['user'],
                                     password=POSTGRES['password'],
                                     database=POSTGRES['database'],
                                     # initializing with 0 connections allows the web server to
                                     ...
```

Discussion:

Patch:
diff --git a/beacon_api/conf/config.py b/beacon_api/conf/config.py
index 832143e..b3aba49 100644
--- a/beacon_api/conf/config.py
+++ b/beacon_api/conf/config.py
@@ -8,19 +8,6 @@ At this point we also initialize a connection pool that the API is going to use
 import os
 import asyncpg
 
-URL = os.environ.get('DATABASE_URL', 'postgresql://localhost:5432').split('/')[2]
-POSTGRES = {
-    'user': os.environ.get('DATABASE_USER', 'beacon'),
-    'password': os.environ.get('DATABASE_PASSWORD', 'beacon'),
-    'database': os.environ.get('DATABASE_NAME', 'beacondb'),
-    'host': URL,
-}
-
-DB_URL = 'postgresql://{user}:{pw}@{url}/{db}'.format(user=POSTGRES['user'],
-                                                      pw=POSTGRES['password'],
-                                                      url=POSTGRES['host'],
-                                                      db=POSTGRES['database'])
-
 DB_SCHEMA = os.environ.get('DATABASE_SCHEMA', '')
 DB_SCHEMA += '.' if DB_SCHEMA else ''
 
@@ -30,7 +17,11 @@ async def init_db_pool():
 
     As we will have frequent requests to the database it is recommended to create a connection pool.
     """
-    return await asyncpg.create_pool(dsn=DB_URL,
+    return await asyncpg.create_pool(host=os.environ.get('DATABASE_URL', 'localhost'),
+                                     port=os.environ.get('DATABASE_PORT', '5432'),
+                                     user=os.environ.get('DATABASE_USER', 'beacon'),
+                                     password=os.environ.get('DATABASE_PASSWORD', 'beacon'),
+                                     database=os.environ.get('DATABASE_NAME', 'beacondb'),
                                      # initializing with 0 connections allows the web server to
                                      # start and also continue to live
                                      min_size=0,
diff --git a/beacon_api/utils/db_load.py b/beacon_api/utils/db_load.py
index 3a9f049..f837616 100644
--- a/beacon_api/utils/db_load.py
+++ b/beacon_api/utils/db_load.py
@@ -47,20 +47,16 @@ from cyvcf2 import VCF
 
 from datetime import datetime
 
-from ..conf.config import DB_URL
 from .logging import LOG
 
 
 class BeaconDB:
     """Database connection and operations."""
 
-    def __init__(self, db_url):
+    def __init__(self):
         """Start database routines."""
         LOG.info('Start database routines')
         self._conn = None
-        LOG.info('Fetch database URL from config')
-        self._db_url = db_url
-        LOG.info('Database URL has been set -> Connections can now be made')
 
     def _transform_vt(self, vt, variant):
         """Transform variant types."""
@@ -131,7 +127,11 @@ class BeaconDB:
         """Connect to the database."""
         LOG.info('Establish a connection to database')
         try:
-            self._conn = await asyncpg.connect(self._db_url)
+            self._conn = await asyncpg.connect(host=os.environ.get('DATABASE_URL', 'localhost'),
+                                               port=os.environ.get('DATABASE_PORT', '5432'),
+                                               user=os.environ.get('DATABASE_USER', 'beacon'),
+                                               password=os.environ.get('DATABASE_PASSWORD', 'beacon'),
+                                               database=os.environ.get('DATABASE_NAME', 'beacondb'))
             LOG.info('Database connection has been established')
         except Exception as e:
             LOG.error(f'AN ERROR OCCURRED WHILE ATTEMPTING TO CONNECT TO DATABASE -> {e}')
@@ -266,7 +266,7 @@ async def init_beacon_db(arguments=None):
     args = parse_arguments(arguments)
 
     # Initialise the database connection
-    db = BeaconDB(DB_URL)
+    db = BeaconDB()
 
     # Connect to the database
     await db.connection()
diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml
index f766395..3e3780c 100644
--- a/deploy/docker-compose.yml
+++ b/deploy/docker-compose.yml
@@ -18,7 +18,7 @@ services:
     depends_on:
       - postgres
     environment:
-      DATABASE_URL: postgresql://postgres:5432
+      DATABASE_URL: postgres
     links:
     - postgres:postgres
     ports:
diff --git a/docs/instructions.rst b/docs/instructions.rst
index c8895b8..b95d387 100644
--- a/docs/instructions.rst
+++ b/docs/instructions.rst
@@ -17,7 +17,9 @@ the table below.
 +---------------------+-------------------------------+--------------------------------------------------+
 | ENV                 | Default                       | Description                                      |
 +---------------------+-------------------------------+--------------------------------------------------+
-| `DATABASE_URL`      | `postgresql://localhost:5432` | The URL for the PostgreSQL server.               |
+| `DATABASE_URL`      | `localhost`                   | The URL for the PostgreSQL server.               |
++---------------------+-------------------------------+--------------------------------------------------+
+| `DATABASE_PORT`     | `5432`                        | The port for the PostgreSQL server.              |
 +---------------------+-------------------------------+--------------------------------------------------+
 | `DATABASE_NAME`     | `beacondb`                    | Name of the database.                            |
 +---------------------+-------------------------------+--------------------------------------------------+
@@ -42,7 +44,8 @@ Setting the necessary environment variables can be done  e.g. via the command li
 
 .. code-block:: console
 
-    $ export DATABASE_URL=postgresql://localhost:5432
+    $ export DATABASE_URL=localhost
+    $ export DATABASE_PORT=5434
     $ export DATABASE_NAME=beacondb
     $ export DATABASE_USER=beacon
     $ export DATABASE_PASSWORD=beacon


Test patch:
diff --git a/tests/test_db_load.py b/tests/test_db_load.py
index 66a0439..ac01147 100644
--- a/tests/test_db_load.py
+++ b/tests/test_db_load.py
@@ -95,8 +95,7 @@ class DatabaseTestCase(asynctest.TestCase):
 
     def setUp(self):
         """Initialise BeaconDB object."""
-        self._db_url = 'http://url.fi'
-        self._db = BeaconDB(self._db_url)
+        self._db = BeaconDB()
         self._dir = TempDirectory()
         self.data = """##fileformat=VCFv4.0
         ##fileDate=20090805
@@ -133,14 +132,14 @@ class DatabaseTestCase(asynctest.TestCase):
     async def test_connection(self, db_mock):
         """Test database URL fetching."""
         await self._db.connection()
-        db_mock.connect.assert_called_with(self._db_url)
+        db_mock.connect.assert_called()
 
     @asynctest.mock.patch('beacon_api.utils.db_load.asyncpg.connect')
     async def test_check_tables(self, db_mock):
         """Test checking tables."""
         db_mock.return_value = Connection()
         await self._db.connection()
-        db_mock.assert_called_with(self._db_url)
+        db_mock.assert_called()
         result = await self._db.check_tables(['DATATSET1', 'DATATSET2'])
         # No Missing tables
         assert result == []
@@ -155,7 +154,7 @@ class DatabaseTestCase(asynctest.TestCase):
             PRIMARY KEY (id));"""
         db_mock.return_value = Connection()
         await self._db.connection()
-        db_mock.assert_called_with(self._db_url)
+        db_mock.assert_called()
         sql_file = self._dir.write('sql.init', sql.encode('utf-8'))
         await self._db.create_tables(sql_file)
         # Should assert logs
@@ -177,7 +176,7 @@ class DatabaseTestCase(asynctest.TestCase):
             "accessType": "PUBLIC"}"""
         db_mock.return_value = Connection()
         await self._db.connection()
-        db_mock.assert_called_with(self._db_url)
+        db_mock.assert_called()
         metafile = self._dir.write('data.json', metadata.encode('utf-8'))
         vcf = asynctest.mock.MagicMock(name='samples')
         vcf.samples.return_value = [1, 2, 3]
@@ -195,7 +194,7 @@ class DatabaseTestCase(asynctest.TestCase):
         vcf.return_value = [{'record': 1}, {'record': 2}, {'records': 3}]
         vcf.samples.return_value = [{'record': 1}, {'record': 2}, {'records': 3}]
         await self._db.connection()
-        db_mock.assert_called_with(self._db_url)
+        db_mock.assert_called()
         await self._db.load_datafile(vcf, self.datafile, 'DATASET1')
         # Should assert logs
         mock_log.info.mock_calls = [f'Read data from {self.datafile}',
@@ -207,16 +206,17 @@ class DatabaseTestCase(asynctest.TestCase):
         """Test load_datafile."""
         db_mock.return_value = Connection()
         await self._db.connection()
-        db_mock.assert_called_with(self._db_url)
+        db_mock.assert_called()
         await self._db.insert_variants('DATASET1', ['C'], 1)
         # Should assert logs
         mock_log.info.mock_calls = [f'Received 1 variants for insertion to DATASET1',
                                     'Insert variants into the database']
 
-    def test_bad_init(self):
-        """Capture error in case of anything wrong with initializing BeaconDB."""
-        with self.assertRaises(TypeError):
-            BeaconDB()
+    # This was the case when BeaconDB() was initiated with a URL parameter, now it happens with environment variables
+    # def test_bad_init(self):
+    #     """Capture error in case of anything wrong with initializing BeaconDB."""
+    #     with self.assertRaises(TypeError):
+    #         BeaconDB()
 
     @asynctest.mock.patch('beacon_api.utils.db_load.LOG')
     @asynctest.mock.patch('beacon_api.utils.db_load.asyncpg.connect')