diff --git a/.travis-postgres10.sh b/.travis-postgres10.sh deleted file mode 100755 index bca31aa..0000000 --- a/.travis-postgres10.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env bash -# -# Installs PostgreSQL 10 on Travis CI, as the default build images don't support it yet. -# -# @see https://github.com/travis-ci/travis-ci/issues/8537#issuecomment-354020356 - -set -euxo pipefail - -echo "Installing Postgres 10..." -sudo service postgresql stop -sudo apt-get remove --quiet 'postgresql-*' -sudo apt-get update --quiet -sudo apt-get install --quiet postgresql-10 postgresql-client-10 -sudo cp /etc/postgresql/{9.6,10}/main/pg_hba.conf - -echo "Restarting Postgres 10..." -sudo service postgresql restart - -echo "Adding Travis role..." -sudo psql --command="CREATE ROLE travis SUPERUSER LOGIN CREATEDB;" --username="postgres" diff --git a/.travis.yml b/.travis.yml index e96d681..4b04d29 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,7 +16,11 @@ services: # @see https://github.com/travis-ci/travis-ci/issues/8537#issuecomment-354020356 sudo: required addons: - postgresql: 9.6 + postgresql: "10" + apt: + packages: + - postgresql-10 + - postgresql-client-10 cache: yarn: true @@ -29,9 +33,6 @@ before_install: - curl --location https://yarnpkg.com/install.sh | bash -s -- --version "$( node -e "console.log(require('./package.json').engines.yarn)" )" - export PATH="$HOME/.yarn/bin:$PATH" - # Upgrade to PostgreSQL 10. - - ./.travis-postgres10.sh - install: - yarn --frozen-lockfile diff --git a/src/points.js b/src/points.js index abc201b..e4c4071 100644 --- a/src/points.js +++ b/src/points.js @@ -73,17 +73,21 @@ const updateScore = async( item, operation ) => { ' ); // Atomically record the action. - // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. - await dbClient.query( '\ - INSERT INTO ' + scoresTableName + ' VALUES (\'' + item + '\', ' + operation + '1) \ - ON CONFLICT (item) DO UPDATE SET score = ' + scoresTableName + '.score ' + operation + ' 1; \ - ' ); + const insertOrUpdateScore = { + text: '\ + INSERT INTO ' + scoresTableName + ' VALUES ($1, $2) \ + ON CONFLICT (item) DO UPDATE SET score = ' + scoresTableName + '.score ' + operation + ' 1; \ + ', + values: [ item, operation + '1' ] + }; + await dbClient.query( insertOrUpdateScore ); // Get the new value. - // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. - const dbSelect = await dbClient.query( '\ - SELECT score FROM ' + scoresTableName + ' WHERE item = \'' + item + '\'; \ - ' ); + const getCurrentScore = { + text: 'SELECT score FROM ' + scoresTableName + ' WHERE item = $1;', + values: [ item ] + }; + const dbSelect = await dbClient.query( getCurrentScore ); await dbClient.release(); const score = dbSelect.rows[0].score; diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 21524cc..ac4c120 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -196,6 +196,12 @@ describe( 'The database', () => { const extensionExistsQuery = 'SELECT * FROM pg_extension WHERE extname = \'citext\''; + // Get the new value. + const getCurrentScore = { + text: 'SELECT score FROM ' + config.scoresTableName + ' WHERE item = $1;', + values: [ defaultItem ] + }; + it( 'does not yet have the ' + config.scoresTableName + ' table', async() => { expect.hasAssertions(); const dbClient = await postgres.connect(); @@ -214,11 +220,14 @@ describe( 'The database', () => { it( 'creates the ' + config.scoresTableName + ' table on the first request', async() => { expect.hasAssertions(); - await points.updateScore( defaultItem, '++' ); + const newScore = await points.updateScore( defaultItem, '+' ); + expect( newScore ).toBe( 1 ); const dbClient = await postgres.connect(); const query = await dbClient.query( tableExistsQuery ); - await dbClient.release(); expect( query.rows[0].exists ).toBeTrue(); + const queryScore = await dbClient.query( getCurrentScore ); + expect( queryScore.rows[0].score ).toBe( 1 ); + await dbClient.release(); }); it( 'also creates the case-insensitive extension on the first request', async() => { @@ -229,14 +238,15 @@ describe( 'The database', () => { expect( query.rowCount ).toBe( 1 ); }); - /* eslint-disable jest/expect-expect */ - // TODO: This test really should have an assertion, but I can't figure out how to catch the error - // properly... it's possible that updateScore needs rewriting to catch properly. In the - // meantime, this test *does* actually work like expected. it( 'does not cause any errors on a second request when everything already exists', async() => { - await points.updateScore( defaultItem, '++' ); + expect.hasAssertions(); + const newScore = await points.updateScore( defaultItem, '+' ); + expect( newScore ).toBe( 2 ); + const dbClient = await postgres.connect(); + const queryScore = await dbClient.query( getCurrentScore ); + expect( queryScore.rows[0].score ).toBe( 2 ); + await dbClient.release(); }); - /* eslint-enable jest/expect-expect */ it( 'returns a list of top scores in the correct order', async() => { expect.hasAssertions(); @@ -253,9 +263,9 @@ describe( 'The database', () => { ]; // Give us a few additional scores so we can check the order works. - await points.updateScore( defaultUser, '++' ); - await points.updateScore( defaultUser, '++' ); - await points.updateScore( defaultUser, '++' ); + await points.updateScore( defaultUser, '+' ); + await points.updateScore( defaultUser, '+' ); + await points.updateScore( defaultUser, '+' ); const topScores = await points.retrieveTopScores(); expect( topScores ).toEqual( expectedScores );