From 0dce9e1379ea867c9a00c8e6cf1d42badb52601d Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Tue, 30 Oct 2018 23:55:43 +1100 Subject: Write some tests for the email pusher (#4095) --- .travis.yml | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index fd41841c77..655fab9d8e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -23,6 +23,9 @@ branches: - develop - /^release-v/ +# When running the tox environments that call Twisted Trial, we can pass the -j +# flag to run the tests concurrently. We set this to 2 for CPU bound tests +# (SQLite) and 4 for I/O bound tests (PostgreSQL). matrix: fast_finish: true include: @@ -33,10 +36,10 @@ matrix: env: TOX_ENV="pep8,check_isort" - python: 2.7 - env: TOX_ENV=py27 + env: TOX_ENV=py27 TRIAL_FLAGS="-j 2" - python: 2.7 - env: TOX_ENV=py27-old + env: TOX_ENV=py27-old TRIAL_FLAGS="-j 2" - python: 2.7 env: TOX_ENV=py27-postgres TRIAL_FLAGS="-j 4" @@ -44,10 +47,10 @@ matrix: - postgresql - python: 3.5 - env: TOX_ENV=py35 + env: TOX_ENV=py35 TRIAL_FLAGS="-j 2" - python: 3.6 - env: TOX_ENV=py36 + env: TOX_ENV=py36 TRIAL_FLAGS="-j 2" - python: 3.6 env: TOX_ENV=py36-postgres TRIAL_FLAGS="-j 4" -- cgit 1.4.1 From 8ca53fb53e1601f83c68a464b3390b4e22f25233 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 28 Nov 2018 20:59:31 +1100 Subject: Report combined coverage to codecov (#4225) --- .travis.yml | 10 +++++----- changelog.d/4225.misc | 1 + tox.ini | 29 +++++++++++++++++------------ 3 files changed, 23 insertions(+), 17 deletions(-) create mode 100644 changelog.d/4225.misc (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index 655fab9d8e..84d5efff9b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -36,24 +36,24 @@ matrix: env: TOX_ENV="pep8,check_isort" - python: 2.7 - env: TOX_ENV=py27 TRIAL_FLAGS="-j 2" + env: TOX_ENV=py27,codecov TRIAL_FLAGS="-j 2" - python: 2.7 env: TOX_ENV=py27-old TRIAL_FLAGS="-j 2" - python: 2.7 - env: TOX_ENV=py27-postgres TRIAL_FLAGS="-j 4" + env: TOX_ENV=py27-postgres,codecov TRIAL_FLAGS="-j 4" services: - postgresql - python: 3.5 - env: TOX_ENV=py35 TRIAL_FLAGS="-j 2" + env: TOX_ENV=py35,codecov TRIAL_FLAGS="-j 2" - python: 3.6 - env: TOX_ENV=py36 TRIAL_FLAGS="-j 2" + env: TOX_ENV=py36,codecov TRIAL_FLAGS="-j 2" - python: 3.6 - env: TOX_ENV=py36-postgres TRIAL_FLAGS="-j 4" + env: TOX_ENV=py36-postgres,codecov TRIAL_FLAGS="-j 4" services: - postgresql diff --git a/changelog.d/4225.misc b/changelog.d/4225.misc new file mode 100644 index 0000000000..39062696ea --- /dev/null +++ b/changelog.d/4225.misc @@ -0,0 +1 @@ +Added automated coverage reporting to CI. diff --git a/tox.ini b/tox.ini index dfd9afdd49..731094b5da 100644 --- a/tox.ini +++ b/tox.ini @@ -7,6 +7,7 @@ deps = mock python-subunit junitxml + coverage # needed by some of the tests lxml @@ -27,11 +28,15 @@ deps = setenv = PYTHONDONTWRITEBYTECODE = no_byte_code + COVERAGE_PROCESS_START = {toxinidir}/.coveragerc [testenv] deps = {[base]deps} +whitelist_externals = + sh + setenv = {[base]setenv} @@ -39,7 +44,9 @@ passenv = * commands = /usr/bin/find "{toxinidir}" -name '*.pyc' -delete - "{envbindir}/trial" {env:TRIAL_FLAGS:} {posargs:tests} {env:TOXSUFFIX:} + # Add this so that coverage will run on subprocesses + sh -c 'echo "import coverage; coverage.process_startup()" > {envsitepackagesdir}/../sitecustomize.py' + {envbindir}/coverage run "{envbindir}/trial" {env:TRIAL_FLAGS:} {posargs:tests} {env:TOXSUFFIX:} [testenv:py27] @@ -101,17 +108,6 @@ usedevelop=true [testenv:py36] usedevelop=true - -[testenv:py36-coverage] -usedevelop=true -deps = - {[base]deps} - coverage -commands = - /usr/bin/find "{toxinidir}" -name '*.pyc' -delete - python -m coverage run -m twisted.trial {env:TRIAL_FLAGS:} {posargs:tests} {env:TOXSUFFIX:} - - [testenv:py36-postgres] usedevelop=true deps = @@ -146,3 +142,12 @@ deps = towncrier>=18.6.0rc1 commands = python -m towncrier.check --compare-with=origin/develop basepython = python3.6 + +[testenv:codecov] +skip_install = True +deps = + coverage + codecov +commands = + coverage combine + codecov -X gcov \ No newline at end of file -- cgit 1.4.1 From a35c66a00bbee0bb6185c4d37914a41c52c83ddf Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Sat, 12 Jan 2019 06:21:50 +1100 Subject: Remove duplicates in the user_ips table and add an index (#4370) --- .travis.yml | 3 + changelog.d/4370.misc | 1 + synapse/storage/client_ips.py | 138 ++++++++++++++++++++- synapse/storage/schema/delta/53/user_ips_index.sql | 26 ++++ 4 files changed, 164 insertions(+), 4 deletions(-) create mode 100644 changelog.d/4370.misc create mode 100644 synapse/storage/schema/delta/53/user_ips_index.sql (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index 84d5efff9b..728f0e248a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,6 +12,9 @@ cache: # - $HOME/.cache/pip/wheels +addons: + postgresql: "9.4" + # don't clone the whole repo history, one commit will do git: depth: 1 diff --git a/changelog.d/4370.misc b/changelog.d/4370.misc new file mode 100644 index 0000000000..047061ed3c --- /dev/null +++ b/changelog.d/4370.misc @@ -0,0 +1 @@ +Apply a unique index to the user_ips table, preventing duplicates. diff --git a/synapse/storage/client_ips.py b/synapse/storage/client_ips.py index 9ad17b7c25..5d548f250a 100644 --- a/synapse/storage/client_ips.py +++ b/synapse/storage/client_ips.py @@ -65,7 +65,27 @@ class ClientIpStore(background_updates.BackgroundUpdateStore): columns=["last_seen"], ) - # (user_id, access_token, ip) -> (user_agent, device_id, last_seen) + self.register_background_update_handler( + "user_ips_remove_dupes", + self._remove_user_ip_dupes, + ) + + # Register a unique index + self.register_background_index_update( + "user_ips_device_unique_index", + index_name="user_ips_user_token_ip_unique_index", + table="user_ips", + columns=["user_id", "access_token", "ip"], + unique=True, + ) + + # Drop the old non-unique index + self.register_background_update_handler( + "user_ips_drop_nonunique_index", + self._remove_user_ip_nonunique, + ) + + # (user_id, access_token, ip,) -> (user_agent, device_id, last_seen) self._batch_row_update = {} self._client_ip_looper = self._clock.looping_call( @@ -75,6 +95,116 @@ class ClientIpStore(background_updates.BackgroundUpdateStore): "before", "shutdown", self._update_client_ips_batch ) + @defer.inlineCallbacks + def _remove_user_ip_nonunique(self, progress, batch_size): + def f(conn): + txn = conn.cursor() + txn.execute( + "DROP INDEX IF EXISTS user_ips_user_ip" + ) + txn.close() + + yield self.runWithConnection(f) + yield self._end_background_update("user_ips_drop_nonunique_index") + defer.returnValue(1) + + @defer.inlineCallbacks + def _remove_user_ip_dupes(self, progress, batch_size): + + last_seen_progress = progress.get("last_seen", 0) + + def get_last_seen(txn): + txn.execute( + """ + SELECT last_seen FROM user_ips + WHERE last_seen > ? + ORDER BY last_seen + LIMIT 1 + OFFSET ? + """, + (last_seen_progress, batch_size) + ) + results = txn.fetchone() + return results + + # Get a last seen that's sufficiently far away enough from the last one + last_seen = yield self.runInteraction( + "user_ips_dups_get_last_seen", get_last_seen + ) + + if not last_seen: + # If we get a None then we're reaching the end and just need to + # delete the last batch. + last = True + + # We fake not having an upper bound by using a future date, by + # just multiplying the current time by two.... + last_seen = int(self.clock.time_msec()) * 2 + else: + last = False + last_seen = last_seen[0] + + def remove(txn, last_seen_progress, last_seen): + # This works by looking at all entries in the given time span, and + # then for each (user_id, access_token, ip) tuple in that range + # checking for any duplicates in the rest of the table (via a join). + # It then only returns entries which have duplicates, and the max + # last_seen across all duplicates, which can the be used to delete + # all other duplicates. + # It is efficient due to the existence of (user_id, access_token, + # ip) and (last_seen) indices. + txn.execute( + """ + SELECT user_id, access_token, ip, + MAX(device_id), MAX(user_agent), MAX(last_seen) + FROM ( + SELECT user_id, access_token, ip + FROM user_ips + WHERE ? <= last_seen AND last_seen < ? + ORDER BY last_seen + ) c + INNER JOIN user_ips USING (user_id, access_token, ip) + GROUP BY user_id, access_token, ip + HAVING count(*) > 1""", + (last_seen_progress, last_seen) + ) + res = txn.fetchall() + + # We've got some duplicates + for i in res: + user_id, access_token, ip, device_id, user_agent, last_seen = i + + # Drop all the duplicates + txn.execute( + """ + DELETE FROM user_ips + WHERE user_id = ? AND access_token = ? AND ip = ? + """, + (user_id, access_token, ip) + ) + + # Add in one to be the last_seen + txn.execute( + """ + INSERT INTO user_ips + (user_id, access_token, ip, device_id, user_agent, last_seen) + VALUES (?, ?, ?, ?, ?, ?) + """, + (user_id, access_token, ip, device_id, user_agent, last_seen) + ) + + self._background_update_progress_txn( + txn, "user_ips_remove_dupes", {"last_seen": last_seen} + ) + + yield self.runInteraction( + "user_ips_dups_remove", remove, last_seen_progress, last_seen + ) + if last: + yield self._end_background_update("user_ips_remove_dupes") + + defer.returnValue(batch_size) + @defer.inlineCallbacks def insert_client_ip(self, user_id, access_token, ip, user_agent, device_id, now=None): @@ -127,10 +257,10 @@ class ClientIpStore(background_updates.BackgroundUpdateStore): "user_id": user_id, "access_token": access_token, "ip": ip, - "user_agent": user_agent, - "device_id": device_id, }, values={ + "user_agent": user_agent, + "device_id": device_id, "last_seen": last_seen, }, lock=False, @@ -227,7 +357,7 @@ class ClientIpStore(background_updates.BackgroundUpdateStore): results = {} for key in self._batch_row_update: - uid, access_token, ip = key + uid, access_token, ip, = key if uid == user_id: user_agent, _, last_seen = self._batch_row_update[key] results[(access_token, ip)] = (user_agent, last_seen) diff --git a/synapse/storage/schema/delta/53/user_ips_index.sql b/synapse/storage/schema/delta/53/user_ips_index.sql new file mode 100644 index 0000000000..4ca346c111 --- /dev/null +++ b/synapse/storage/schema/delta/53/user_ips_index.sql @@ -0,0 +1,26 @@ +/* Copyright 2018 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +-- delete duplicates +INSERT INTO background_updates (update_name, progress_json) VALUES + ('user_ips_remove_dupes', '{}'); + +-- add a new unique index to user_ips table +INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES + ('user_ips_device_unique_index', '{}', 'user_ips_remove_dupes'); + +-- drop the old original index +INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES + ('user_ips_drop_nonunique_index', '{}', 'user_ips_device_unique_index'); \ No newline at end of file -- cgit 1.4.1 From 34b25dcc8ef0d9012eb3b0d9a158e11b159ffce0 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 11 Jan 2019 19:22:56 +0000 Subject: Silence travis-ci build warnings by removing non-functional python3.6 (#4377) * Remove non-functional python3.6 in travis env * changelog --- .travis.yml | 7 +++++++ changelog.d/4377.misc | 1 + 2 files changed, 8 insertions(+) create mode 100644 changelog.d/4377.misc (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index 728f0e248a..3cab77ce4d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -71,6 +71,13 @@ matrix: install: - pip install tox + + # if we don't have python3.6 in this environment, travis unhelpfully gives us + # a `python3.6` on our path which does nothing but spit out a warning. Tox + # tries to run it (even if we're not running a py36 env), so the build logs + # then have warnings which look like errors. To reduce the noise, remove the + # non-functional python3.6. + - ( ! command -v python3.6 || python3.6 --version ) &>/dev/null || rm -f $(command -v python3.6) script: - tox -e $TOX_ENV diff --git a/changelog.d/4377.misc b/changelog.d/4377.misc new file mode 100644 index 0000000000..06273023fc --- /dev/null +++ b/changelog.d/4377.misc @@ -0,0 +1 @@ +Silence travis-ci build warnings by removing non-functional python3.6 \ No newline at end of file -- cgit 1.4.1 From 0869f01e74b356e2f5f617a61aa6e0b6b2c08f79 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 19 Feb 2019 09:52:03 +0000 Subject: Test against Postgres 9.5 as well as 9.4 Postgres 9.5 is the first to support UPSERTs, so we should really run against it as well as 9.4. --- .travis.yml | 46 ++++++++++++++++++++++++++++++++-------------- changelog.d/4676.misc | 1 + 2 files changed, 33 insertions(+), 14 deletions(-) create mode 100644 changelog.d/4676.misc (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index 3cab77ce4d..f6c91c2621 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,9 +12,6 @@ cache: # - $HOME/.cache/pip/wheels -addons: - postgresql: "9.4" - # don't clone the whole repo history, one commit will do git: depth: 1 @@ -25,6 +22,7 @@ branches: - master - develop - /^release-v/ + - rav/pg95 # When running the tox environments that call Twisted Trial, we can pass the -j # flag to run the tests concurrently. We set this to 2 for CPU bound tests @@ -32,36 +30,53 @@ branches: matrix: fast_finish: true include: - - python: 2.7 - env: TOX_ENV=packaging - - - python: 3.6 - env: TOX_ENV="pep8,check_isort" + - name: "pep8" + python: 3.6 + env: TOX_ENV="pep8,check_isort,packaging" - - python: 2.7 + - name: "py2.7 / sqlite" + python: 2.7 env: TOX_ENV=py27,codecov TRIAL_FLAGS="-j 2" - - python: 2.7 + - name: "py2.7 / sqlite / olddeps" + python: 2.7 env: TOX_ENV=py27-old TRIAL_FLAGS="-j 2" - - python: 2.7 + - name: "py2.7 / postgres9.5" + python: 2.7 + addons: + postgresql: "9.5" env: TOX_ENV=py27-postgres,codecov TRIAL_FLAGS="-j 4" services: - postgresql - - python: 3.5 + - name: "py3.5 / sqlite" + python: 3.5 env: TOX_ENV=py35,codecov TRIAL_FLAGS="-j 2" - - python: 3.6 + - name: "py3.6 / sqlite" + python: 3.6 env: TOX_ENV=py36,codecov TRIAL_FLAGS="-j 2" - - python: 3.6 + - name: "py3.6 / postgres9.4" + python: 3.6 + addons: + postgresql: "9.4" + env: TOX_ENV=py36-postgres TRIAL_FLAGS="-j 4" + services: + - postgresql + + - name: "py3.6 / postgres9.5" + python: 3.6 + addons: + postgresql: "9.5" env: TOX_ENV=py36-postgres,codecov TRIAL_FLAGS="-j 4" services: - postgresql - # we only need to check for the newsfragment if it's a PR build if: type = pull_request + name: "check-newsfragment" python: 3.6 env: TOX_ENV=check-newsfragment script: @@ -70,6 +85,9 @@ matrix: - tox -e $TOX_ENV install: + # this just logs the postgres version we will be testing against (if any) + - psql -At -U postgres -c 'select version();' + - pip install tox # if we don't have python3.6 in this environment, travis unhelpfully gives us diff --git a/changelog.d/4676.misc b/changelog.d/4676.misc new file mode 100644 index 0000000000..a250558e69 --- /dev/null +++ b/changelog.d/4676.misc @@ -0,0 +1 @@ +Test against Postgres 9.5 as well as 9.4 -- cgit 1.4.1 From c594cc8076b34ba79311a51590cb3876fd3a301e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 20 Feb 2019 07:42:24 +0000 Subject: Run unit tests against python 3.7 (#4677) * Run unit tests against python 3.7 ... so that we span the full range of our supported python versions * Switch to xenial * fix psql fail * pep8 etc want python 3.6 --- .travis.yml | 22 +++++++++++----------- changelog.d/4677.misc | 1 + tox.ini | 29 ++--------------------------- 3 files changed, 14 insertions(+), 38 deletions(-) create mode 100644 changelog.d/4677.misc (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index f6c91c2621..d88f10324f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,4 +1,4 @@ -sudo: false +dist: xenial language: python cache: @@ -54,23 +54,23 @@ matrix: python: 3.5 env: TOX_ENV=py35,codecov TRIAL_FLAGS="-j 2" - - name: "py3.6 / sqlite" - python: 3.6 - env: TOX_ENV=py36,codecov TRIAL_FLAGS="-j 2" + - name: "py3.7 / sqlite" + python: 3.7 + env: TOX_ENV=py37,codecov TRIAL_FLAGS="-j 2" - - name: "py3.6 / postgres9.4" - python: 3.6 + - name: "py3.7 / postgres9.4" + python: 3.7 addons: postgresql: "9.4" - env: TOX_ENV=py36-postgres TRIAL_FLAGS="-j 4" + env: TOX_ENV=py37-postgres TRIAL_FLAGS="-j 4" services: - postgresql - - name: "py3.6 / postgres9.5" - python: 3.6 + - name: "py3.7 / postgres9.5" + python: 3.7 addons: postgresql: "9.5" - env: TOX_ENV=py36-postgres,codecov TRIAL_FLAGS="-j 4" + env: TOX_ENV=py37-postgres,codecov TRIAL_FLAGS="-j 4" services: - postgresql @@ -86,7 +86,7 @@ matrix: install: # this just logs the postgres version we will be testing against (if any) - - psql -At -U postgres -c 'select version();' + - psql -At -U postgres -c 'select version();' || true - pip install tox diff --git a/changelog.d/4677.misc b/changelog.d/4677.misc new file mode 100644 index 0000000000..6f4596be4a --- /dev/null +++ b/changelog.d/4677.misc @@ -0,0 +1 @@ +Run unit tests against python 3.7. diff --git a/tox.ini b/tox.ini index 3e2dba2925..14437e7334 100644 --- a/tox.ini +++ b/tox.ini @@ -3,7 +3,6 @@ envlist = packaging, py27, py36, pep8, check_isort [base] deps = - Twisted>=17.1 mock python-subunit junitxml @@ -38,6 +37,7 @@ whitelist_externals = setenv = {[base]setenv} + postgres: SYNAPSE_POSTGRES = 1 passenv = * @@ -47,8 +47,6 @@ commands = sh -c 'echo "import coverage; coverage.process_startup()" > {envsitepackagesdir}/../sitecustomize.py' {envbindir}/coverage run "{envbindir}/trial" {env:TRIAL_FLAGS:} {posargs:tests} {env:TOXSUFFIX:} -[testenv:py27] - # As of twisted 16.4, trial tries to import the tests as a package (previously # it loaded the files explicitly), which means they need to be on the # pythonpath. Our sdist doesn't include the 'tests' package, so normally it @@ -72,14 +70,7 @@ commands = # ) usedevelop=true -[testenv:py27-postgres] -usedevelop=true -deps = - {[base]deps} - psycopg2 -setenv = - {[base]setenv} - SYNAPSE_POSTGRES = 1 + # A test suite for the oldest supported versions of Python libraries, to catch # any uses of APIs not available in them. @@ -101,22 +92,6 @@ commands = pip install -e . {envbindir}/trial {env:TRIAL_FLAGS:} {posargs:tests} {env:TOXSUFFIX:} -[testenv:py35] -usedevelop=true - -[testenv:py36] -usedevelop=true - -[testenv:py36-postgres] -usedevelop=true -deps = - {[base]deps} - psycopg2 -setenv = - {[base]setenv} - SYNAPSE_POSTGRES = 1 - - [testenv:packaging] skip_install=True deps = -- cgit 1.4.1 From a06614bd2aba5d751e002433c88e8ba1ba02c50b Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 20 Feb 2019 23:03:30 +1100 Subject: UPSERT many functionality (#4644) --- .travis.yml | 2 +- changelog.d/4644.misc | 1 + synapse/storage/_base.py | 146 ++++++++++++++++++++++++++++++++++++++++---- tests/storage/test__base.py | 88 ++++++++++++++++++++++++++ 4 files changed, 224 insertions(+), 13 deletions(-) create mode 100644 changelog.d/4644.misc (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index d88f10324f..5d763123a0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -89,7 +89,7 @@ install: - psql -At -U postgres -c 'select version();' || true - pip install tox - + # if we don't have python3.6 in this environment, travis unhelpfully gives us # a `python3.6` on our path which does nothing but spit out a warning. Tox # tries to run it (even if we're not running a py36 env), so the build logs diff --git a/changelog.d/4644.misc b/changelog.d/4644.misc new file mode 100644 index 0000000000..84137c3412 --- /dev/null +++ b/changelog.d/4644.misc @@ -0,0 +1 @@ +Introduce upsert batching functionality in the database layer. diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index f1a5366b95..3d895da43c 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -106,6 +106,14 @@ class LoggingTransaction(object): def __iter__(self): return self.txn.__iter__() + def execute_batch(self, sql, args): + if isinstance(self.database_engine, PostgresEngine): + from psycopg2.extras import execute_batch + self._do_execute(lambda *x: execute_batch(self.txn, *x), sql, args) + else: + for val in args: + self.execute(sql, val) + def execute(self, sql, *args): self._do_execute(self.txn.execute, sql, *args) @@ -699,20 +707,34 @@ class SQLBaseStore(object): else: return "%s = ?" % (key,) - # First try to update. - sql = "UPDATE %s SET %s WHERE %s" % ( - table, - ", ".join("%s = ?" % (k,) for k in values), - " AND ".join(_getwhere(k) for k in keyvalues) - ) - sqlargs = list(values.values()) + list(keyvalues.values()) + if not values: + # If `values` is empty, then all of the values we care about are in + # the unique key, so there is nothing to UPDATE. We can just do a + # SELECT instead to see if it exists. + sql = "SELECT 1 FROM %s WHERE %s" % ( + table, + " AND ".join(_getwhere(k) for k in keyvalues) + ) + sqlargs = list(keyvalues.values()) + txn.execute(sql, sqlargs) + if txn.fetchall(): + # We have an existing record. + return False + else: + # First try to update. + sql = "UPDATE %s SET %s WHERE %s" % ( + table, + ", ".join("%s = ?" % (k,) for k in values), + " AND ".join(_getwhere(k) for k in keyvalues) + ) + sqlargs = list(values.values()) + list(keyvalues.values()) - txn.execute(sql, sqlargs) - if txn.rowcount > 0: - # successfully updated at least one row. - return False + txn.execute(sql, sqlargs) + if txn.rowcount > 0: + # successfully updated at least one row. + return False - # We didn't update any rows so insert a new one + # We didn't find any existing rows, so insert a new one allvalues = {} allvalues.update(keyvalues) allvalues.update(values) @@ -759,6 +781,106 @@ class SQLBaseStore(object): ) txn.execute(sql, list(allvalues.values())) + def _simple_upsert_many_txn( + self, txn, table, key_names, key_values, value_names, value_values + ): + """ + Upsert, many times. + + Args: + table (str): The table to upsert into + key_names (list[str]): The key column names. + key_values (list[list]): A list of each row's key column values. + value_names (list[str]): The value column names. If empty, no + values will be used, even if value_values is provided. + value_values (list[list]): A list of each row's value column values. + Returns: + None + """ + if ( + self.database_engine.can_native_upsert + and table not in self._unsafe_to_upsert_tables + ): + return self._simple_upsert_many_txn_native_upsert( + txn, table, key_names, key_values, value_names, value_values + ) + else: + return self._simple_upsert_many_txn_emulated( + txn, table, key_names, key_values, value_names, value_values + ) + + def _simple_upsert_many_txn_emulated( + self, txn, table, key_names, key_values, value_names, value_values + ): + """ + Upsert, many times, but without native UPSERT support or batching. + + Args: + table (str): The table to upsert into + key_names (list[str]): The key column names. + key_values (list[list]): A list of each row's key column values. + value_names (list[str]): The value column names. If empty, no + values will be used, even if value_values is provided. + value_values (list[list]): A list of each row's value column values. + Returns: + None + """ + # No value columns, therefore make a blank list so that the following + # zip() works correctly. + if not value_names: + value_values = [() for x in range(len(key_values))] + + for keyv, valv in zip(key_values, value_values): + _keys = {x: y for x, y in zip(key_names, keyv)} + _vals = {x: y for x, y in zip(value_names, valv)} + + self._simple_upsert_txn_emulated(txn, table, _keys, _vals) + + def _simple_upsert_many_txn_native_upsert( + self, txn, table, key_names, key_values, value_names, value_values + ): + """ + Upsert, many times, using batching where possible. + + Args: + table (str): The table to upsert into + key_names (list[str]): The key column names. + key_values (list[list]): A list of each row's key column values. + value_names (list[str]): The value column names. If empty, no + values will be used, even if value_values is provided. + value_values (list[list]): A list of each row's value column values. + Returns: + None + """ + allnames = [] + allnames.extend(key_names) + allnames.extend(value_names) + + if not value_names: + # No value columns, therefore make a blank list so that the + # following zip() works correctly. + latter = "NOTHING" + value_values = [() for x in range(len(key_values))] + else: + latter = ( + "UPDATE SET " + ", ".join(k + "=EXCLUDED." + k for k in value_names) + ) + + sql = "INSERT INTO %s (%s) VALUES (%s) ON CONFLICT (%s) DO %s" % ( + table, + ", ".join(k for k in allnames), + ", ".join("?" for _ in allnames), + ", ".join(key_names), + latter, + ) + + args = [] + + for x, y in zip(key_values, value_values): + args.append(tuple(x) + tuple(y)) + + return txn.execute_batch(sql, args) + def _simple_select_one(self, table, keyvalues, retcols, allow_none=False, desc="_simple_select_one"): """Executes a SELECT query on the named table, which is expected to diff --git a/tests/storage/test__base.py b/tests/storage/test__base.py index 52eb05bfbf..dd49a14524 100644 --- a/tests/storage/test__base.py +++ b/tests/storage/test__base.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # Copyright 2015, 2016 OpenMarket Ltd +# Copyright 2019 New Vector Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -314,3 +315,90 @@ class CacheDecoratorTestCase(unittest.TestCase): self.assertEquals(callcount[0], 2) self.assertEquals(callcount2[0], 3) + + +class UpsertManyTests(unittest.HomeserverTestCase): + def prepare(self, reactor, clock, hs): + self.storage = hs.get_datastore() + + self.table_name = "table_" + hs.get_secrets().token_hex(6) + self.get_success( + self.storage.runInteraction( + "create", + lambda x, *a: x.execute(*a), + "CREATE TABLE %s (id INTEGER, username TEXT, value TEXT)" + % (self.table_name,), + ) + ) + self.get_success( + self.storage.runInteraction( + "index", + lambda x, *a: x.execute(*a), + "CREATE UNIQUE INDEX %sindex ON %s(id, username)" + % (self.table_name, self.table_name), + ) + ) + + def _dump_to_tuple(self, res): + for i in res: + yield (i["id"], i["username"], i["value"]) + + def test_upsert_many(self): + """ + Upsert_many will perform the upsert operation across a batch of data. + """ + # Add some data to an empty table + key_names = ["id", "username"] + value_names = ["value"] + key_values = [[1, "user1"], [2, "user2"]] + value_values = [["hello"], ["there"]] + + self.get_success( + self.storage.runInteraction( + "test", + self.storage._simple_upsert_many_txn, + self.table_name, + key_names, + key_values, + value_names, + value_values, + ) + ) + + # Check results are what we expect + res = self.get_success( + self.storage._simple_select_list( + self.table_name, None, ["id, username, value"] + ) + ) + self.assertEqual( + set(self._dump_to_tuple(res)), + set([(1, "user1", "hello"), (2, "user2", "there")]), + ) + + # Update only user2 + key_values = [[2, "user2"]] + value_values = [["bleb"]] + + self.get_success( + self.storage.runInteraction( + "test", + self.storage._simple_upsert_many_txn, + self.table_name, + key_names, + key_values, + value_names, + value_values, + ) + ) + + # Check results are what we expect + res = self.get_success( + self.storage._simple_select_list( + self.table_name, None, ["id, username, value"] + ) + ) + self.assertEqual( + set(self._dump_to_tuple(res)), + set([(1, "user1", "hello"), (2, "user2", "bleb")]), + ) -- cgit 1.4.1 From e1666af9be6b605a6e686a8d9a44347c06175750 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 22 Feb 2019 10:56:59 +0000 Subject: Better checks on newsfragments (#4698) * You need an entry in the debian changelog (and not a regular newsfragment) for debian packaging changes. * Regular newsfragments must end in full stops. --- .travis.yml | 6 +----- CONTRIBUTING.rst | 37 ++++++++++++++++++++++++++++++------- changelog.d/4698.misc | 1 + scripts-dev/check-newsfragment | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 12 deletions(-) create mode 100644 changelog.d/4698.misc create mode 100755 scripts-dev/check-newsfragment (limited to '.travis.yml') diff --git a/.travis.yml b/.travis.yml index 5d763123a0..0d0fa7082a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -78,11 +78,7 @@ matrix: if: type = pull_request name: "check-newsfragment" python: 3.6 - env: TOX_ENV=check-newsfragment - script: - - git remote set-branches --add origin develop - - git fetch origin develop - - tox -e $TOX_ENV + script: scripts-dev/check-newsfragment install: # this just logs the postgres version we will be testing against (if any) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index b99a022c67..9a283ced6e 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -30,7 +30,7 @@ use github's pull request workflow to review the contribution, and either ask you to make any refinements needed or merge it and make them ourselves. The changes will then land on master when we next do a release. -We use `CircleCI `_ and `Travis CI +We use `CircleCI `_ and `Travis CI `_ for continuous integration. All pull requests to synapse get automatically tested by Travis and CircleCI. If your change breaks the build, this will be shown in GitHub, so please @@ -74,16 +74,39 @@ entry. These are managed by Towncrier To create a changelog entry, make a new file in the ``changelog.d`` file named in the format of ``PRnumber.type``. The type can be one of ``feature``, ``bugfix``, ``removal`` (also used for -deprecations), or ``misc`` (for internal-only changes). The content of -the file is your changelog entry, which can contain Markdown -formatting. Adding credits to the changelog is encouraged, we value -your contributions and would like to have you shouted out in the -release notes! +deprecations), or ``misc`` (for internal-only changes). + +The content of the file is your changelog entry, which can contain Markdown +formatting. The entry should end with a full stop ('.') for consistency. + +Adding credits to the changelog is encouraged, we value your +contributions and would like to have you shouted out in the release notes! For example, a fix in PR #1234 would have its changelog entry in ``changelog.d/1234.bugfix``, and contain content like "The security levels of Florbs are now validated when recieved over federation. Contributed by Jane -Matrix". +Matrix.". + +Debian changelog +---------------- + +Changes which affect the debian packaging files (in ``debian``) are an +exception. + +In this case, you will need to add an entry to the debian changelog for the +next release. For this, run the following command:: + + dch + +This will make up a new version number (if there isn't already an unreleased +version in flight), and open an editor where you can add a new changelog entry. +(Our release process will ensure that the version number and maintainer name is +corrected for the release.) + +If your change affects both the debian packaging *and* files outside the debian +directory, you will need both a regular newsfragment *and* an entry in the +debian changelog. (Though typically such changes should be submitted as two +separate pull requests.) Attribution ~~~~~~~~~~~ diff --git a/changelog.d/4698.misc b/changelog.d/4698.misc new file mode 100644 index 0000000000..d17b19bec5 --- /dev/null +++ b/changelog.d/4698.misc @@ -0,0 +1 @@ +Better checks on newsfragments diff --git a/scripts-dev/check-newsfragment b/scripts-dev/check-newsfragment new file mode 100755 index 0000000000..5da093e168 --- /dev/null +++ b/scripts-dev/check-newsfragment @@ -0,0 +1,36 @@ +#!/bin/bash +# +# A script which checks that an appropriate news file has been added on this +# branch. + +set -e + +# make sure that origin/develop is up to date +git fetch origin develop + +UPSTREAM=origin/develop + +# if there are changes in the debian directory, check that the debian changelog +# has been updated +if ! git diff --quiet $UPSTREAM... -- debian; then + if git diff --quiet $UPSTREAM... -- debian/changelog; then + echo "Updates to debian directory, but no update to the changelog." >&2 + exit 1 + fi +fi + +# if there are changes *outside* the debian directory, check that the +# newsfragments have been updated. +if git diff --name-only $UPSTREAM... | grep -qv '^develop/'; then + tox -e check-newsfragment +fi + +# check that any new newsfiles on this branch end with a full stop. +for f in git diff --name-only $UPSTREAM... -- changelog.d; do + lastchar=`tr -d '\n' < $f | tail -c 1` + if [ $lastchar != '.' ]; then + echo "Newsfragment $f does not end with a '.'" >&2 + exit 1 + fi +done + -- cgit 1.4.1