diff --git a/deployment.md b/deployment.md index 21b8ff9c..a5f90177 100644 --- a/deployment.md +++ b/deployment.md @@ -52,6 +52,8 @@ Optional env vars (all have sensible defaults): | `MONTAGE_ROOT_PATH` | `/` | URL root path | | `MONTAGE_LABS_DB` | `true` | Enable Wikireplica queries | | `MONTAGE_FEEL_LOG_PATH` | _(none)_ | Path for feel log | +| `MONTAGE_COMMONS_DB_HOST` | `commonswiki.analytics.db.svc.wikimedia.cloud` | Wikireplica host for the other Commons tables (file, filerevision, actor, ...); before this was the legacy alias `commonswiki.labsdb`, which points to the same s4 analytics replica | +| `MONTAGE_COMMONS_LINKS_DB_HOST` | `links.commonswiki.analytics.db.svc.wikimedia.cloud` | Wikireplica host for the Commons links tables (categorylinks, linktarget, page), on their own cluster since 2026-09-08 ([Wikitech](https://wikitech.wikimedia.org/wiki/News/2026_Commons_links_tables_database_split)) | #### 4. Create the database @@ -104,8 +106,13 @@ If upgrading an existing deployment, run the migration SQL instead (on the basti ```bash mariadb --defaults-file=~/replica.my.cnf -h tools.db.svc.wikimedia.cloud < tools/migrate_prod_db.sql +mariadb --defaults-file=~/replica.my.cnf -h tools.db.svc.wikimedia.cloud < tools/migrate_round_sources_params.sql ``` +`migrate_round_sources_params.sql` widens `round_sources.params` to MEDIUMTEXT. It can run before or +after the deploy, but until it has run, file-list imports whose names pass 64 KB (roughly 1,000+ +names) still fail. + #### 7. Start the service ```bash @@ -253,6 +260,7 @@ while). Run the migration — do **not** "recreate the database", which would wi ```bash mariadb --defaults-file=~/replica.my.cnf -h tools.db.svc.wikimedia.cloud < tools/migrate_prod_db.sql +mariadb --defaults-file=~/replica.my.cnf -h tools.db.svc.wikimedia.cloud < tools/migrate_round_sources_params.sql toolforge webservice buildservice restart --mount all ``` diff --git a/montage/admin_endpoints.py b/montage/admin_endpoints.py index 6b918fba..a8faabfc 100644 --- a/montage/admin_endpoints.py +++ b/montage/admin_endpoints.py @@ -350,7 +350,9 @@ def import_entries(user_dao, round_id, request_dict): params = {'csv_url': csv_url} if warnings: msg = u'unable to load {} files ({!r})'.format(len(warnings), warnings) - import_warnings.append(msg) + # a dict like the other warnings: the frontend shows one value + # per warning, and showed only the last character of a string + import_warnings.append({'import issues': msg}) elif import_method == CATEGORY_METHOD: cat_name = request_dict['category'] entries = coord_dao.add_entries_from_cat(round_id, cat_name) @@ -369,7 +371,7 @@ def import_entries(user_dao, round_id, request_dict): u'- {}'.format(warning) for warning in warnings ]) msg = u'unable to load {} files:\n{}'.format(len(warnings), formatted_warnings) - import_warnings.append({'import issues', msg}) + import_warnings.append({'import issues': msg}) params = {'file_names': file_names} else: raise NotImplementedResponse() diff --git a/montage/labs.py b/montage/labs.py index 374994cb..dd6e6e18 100644 --- a/montage/labs.py +++ b/montage/labs.py @@ -1,5 +1,6 @@ from __future__ import absolute_import import os +from contextlib import contextmanager try: import pymysql @@ -46,24 +47,41 @@ class MissingMySQLClient(RuntimeError): pass -def fetchall_from_commonswiki(query, params): +# Wikireplica hosts, both on the analytics replicas and both overridable, +# e.g. when Wikimedia moves tables again. COMMONS_DB_HOST used to be the +# legacy alias commonswiki.labsdb, which points to the same s4 analytics +# replica. +COMMONS_DB_HOST = os.environ.get( + 'MONTAGE_COMMONS_DB_HOST', + 'commonswiki.analytics.db.svc.wikimedia.cloud') +# Since 2026-09-08 the Commons links tables (categorylinks, linktarget, ...) +# live on their own cluster (x4); the copies on the main Commons replica are +# no longer written, so category membership must be read from this replica. +# See https://wikitech.wikimedia.org/wiki/News/2026_Commons_links_tables_database_split +COMMONS_LINKS_DB_HOST = os.environ.get( + 'MONTAGE_COMMONS_LINKS_DB_HOST', + 'links.commonswiki.analytics.db.svc.wikimedia.cloud') + +# file names per query when looking up a category's files on the main replica +FILE_LOOKUP_CHUNK_SIZE = 500 + + +def _connect(db_host): if pymysql is None: raise MissingMySQLClient('could not import pymysql, check your' ' environment and restart the service') - db_title = 'commonswiki_p' - db_host = 'commonswiki.labsdb' - connection = pymysql.connect(db=db_title, - host=db_host, - read_default_file=DB_CONFIG, - charset='utf8') - cursor = connection.cursor(pymysql.cursors.DictCursor) - cursor.execute(query, params) - res = cursor.fetchall() + return pymysql.connect(db='commonswiki_p', + host=db_host, + read_default_file=DB_CONFIG, + charset='utf8', + cursorclass=pymysql.cursors.DictCursor) + +def _decode_rows(rows): # looking at the schema on labs, it's all varbinary, not varchar, # so this block converts values ret = [] - for rec in res: + for rec in rows: new_rec = {} for k, v in rec.items(): if isinstance(v, bytes): @@ -73,7 +91,68 @@ def fetchall_from_commonswiki(query, params): return ret -def get_files(category_name): +@contextmanager +def commonswiki_connection(db_host=COMMONS_DB_HOST): + """One replica connection, closed on exit. Yields fetchall(query, + params), which runs a query on that connection and returns decoded + dict rows, so a multi-query lookup reuses one connection.""" + connection = _connect(db_host) + + def fetchall(query, params): + cursor = connection.cursor() + try: + cursor.execute(query, params) + return _decode_rows(cursor.fetchall()) + finally: + cursor.close() + + try: + yield fetchall + finally: + connection.close() + + +def fetchall_from_commonswiki(query, params, db_host=COMMONS_DB_HOST): + """Run one query on its own (closed afterwards) connection.""" + with commonswiki_connection(db_host) as fetchall: + return fetchall(query, params) + + +def get_category_file_names(category_name): + """Names of the files directly in a Commons category, read from the + links replica (page exists on both clusters, so this stays one query). + """ + with commonswiki_connection(COMMONS_LINKS_DB_HOST) as fetchall: + return _category_file_names(fetchall, category_name) + + +def _category_file_names(fetchall, category_name): + query = ''' + SELECT DISTINCT page_title AS file_name + FROM categorylinks + JOIN linktarget ON cl_target_id = lt_id + AND lt_namespace = 14 + AND lt_title = %s + JOIN page ON page_id = cl_from + AND page_namespace = 6 + WHERE cl_type = 'file' + ''' + params = (category_name.replace(' ', '_'),) + rows = fetchall(query, params) + return [row['file_name'] for row in rows] + + +def get_files_by_name(file_names): + """File details for file names (underscored), from the main replica.""" + if not file_names: + return [] + with commonswiki_connection(COMMONS_DB_HOST) as fetchall: + return _files_by_name(fetchall, file_names) + + +def _files_by_name(fetchall, file_names): + if not file_names: + return [] query = ''' SELECT DISTINCT {cols} FROM commonswiki_p.file AS file @@ -82,20 +161,44 @@ def get_files(category_name): LEFT JOIN actor AS ci ON fr.fr_actor = ci.actor_id LEFT JOIN commonswiki_p.filetypes AS ft ON file.file_type = ft.ft_id {earliest_rev} - JOIN page ON page_namespace = 6 - AND page_title = file.file_name - JOIN categorylinks ON cl_from = page_id - AND cl_type = 'file' - JOIN linktarget ON cl_target_id = lt_id - AND lt_namespace = 14 - AND lt_title = %s - WHERE file.file_deleted = 0 - ORDER BY file.file_name ASC + WHERE file.file_name IN ({names}) + AND file.file_deleted = 0 '''.format(cols=', '.join(FILE_COLS), - earliest_rev=_EARLIEST_REVISION_SUBQUERY) - params = (category_name.replace(' ', '_'),) + earliest_rev=_EARLIEST_REVISION_SUBQUERY, + names=', '.join(['%s'] * len(file_names))) + return fetchall(query, tuple(file_names)) + + +def get_files_info_by_names(file_names): + """{underscored name: file info} for the names that exist, looked up in + chunks instead of one query per file, all on one connection.""" + names = sorted(set(name.replace(' ', '_') for name in file_names)) + ret = {} + if not names: + return ret + with commonswiki_connection(COMMONS_DB_HOST) as fetchall: + for i in range(0, len(names), FILE_LOOKUP_CHUNK_SIZE): + chunk = names[i:i + FILE_LOOKUP_CHUNK_SIZE] + for rec in _files_by_name(fetchall, chunk): + ret[rec['img_name']] = rec + return ret - return fetchall_from_commonswiki(query, params) + +def get_files(category_name): + # Two steps because category membership and file data now live on + # different database clusters, which cannot be joined in SQL. + # One connection per cluster for the whole lookup. + file_names = sorted(set(get_category_file_names(category_name))) + ret = [] + if not file_names: + return ret + with commonswiki_connection(COMMONS_DB_HOST) as fetchall: + for i in range(0, len(file_names), FILE_LOOKUP_CHUNK_SIZE): + chunk = file_names[i:i + FILE_LOOKUP_CHUNK_SIZE] + ret.extend(_files_by_name(fetchall, chunk)) + # same order as the old single query's ORDER BY file_name (binary) + ret.sort(key=lambda rec: rec['img_name'].encode('utf8')) + return ret def get_file_info(filename): @@ -119,51 +222,6 @@ def get_file_info(filename): return None -def get_files_legacy(category_name): - """Verbatim copy of the original get_files() using image/oldimage tables. - - Kept alive solely for the xfail parity test (test_get_files_parity). - Remove together with that test after 28 May 2026 once image/oldimage are - dropped from wikireplicas. - """ - IMAGE_COLS = ['img_width', - 'img_height', - 'img_name', - 'img_major_mime', - 'img_minor_mime', - 'IFNULL(oi.actor_user, ci.actor_user) AS img_user', - 'IFNULL(oi.actor_name, ci.actor_name) AS img_user_text', - 'IFNULL(oi_timestamp, img_timestamp) AS img_timestamp', - 'img_timestamp AS rec_img_timestamp', - 'ci.actor_user AS rec_img_user', - 'ci.actor_name AS rec_img_text', - 'oi.oi_archive_name AS oi_archive_name'] - query = ''' - SELECT {cols} - FROM commonswiki_p.image AS i - LEFT JOIN actor AS ci ON img_actor=ci.actor_id - LEFT JOIN (SELECT oi_name, - oi_actor, - actor_user, - actor_name, - oi_timestamp, - oi_archive_name - FROM oldimage - LEFT JOIN actor ON oi_actor=actor.actor_id) AS oi ON img_name=oi.oi_name - JOIN page ON page_namespace = 6 - AND page_title = img_name - JOIN categorylinks ON cl_from = page_id - AND cl_type = 'file' - JOIN linktarget ON cl_target_id = lt_id - AND lt_namespace = 14 - AND lt_title = %s - GROUP BY img_name - ORDER BY oi_timestamp ASC; - '''.format(cols=', '.join(IMAGE_COLS)) - params = (category_name.replace(' ', '_'),) - return fetchall_from_commonswiki(query, params) - - if __name__ == '__main__': imgs = get_files('Images_from_Wiki_Loves_Monuments_2015_in_France') print(imgs) diff --git a/montage/loaders.py b/montage/loaders.py index c10e826f..8a53b561 100644 --- a/montage/loaders.py +++ b/montage/loaders.py @@ -10,7 +10,7 @@ from unicodecsv import DictReader import montage.rdb # TODO: circular import -from .labs import get_files, get_file_info +from .labs import get_files, get_files_info_by_names from .utils import unicode, requests_get, requests_post REMOTE_UTILS_URL = 'https://montage.toolforge.org/v1/utils/' @@ -137,11 +137,7 @@ def load_name_list(file_obj, source='local'): if source == 'remote': edicts, warnings = get_by_filename_remote(rl) else: - for filename in rl: - file_info = get_file_info(filename) - if file_info is not None: - edict, warnings = file_info - edicts.append(edict) + edicts, warnings = _lookup_file_names(rl) for edict in edicts: try: @@ -196,22 +192,33 @@ def get_entries_from_gsheet(raw_url, source='local'): return ret, warnings +def _lookup_file_names(filenames): + """File infos in input order, plus a warning per name that was not found. + + Looks the names up in batches: one query per file made large file-list + imports outlast the request timeout. + """ + found = get_files_info_by_names(filenames) + files, warnings = [], [] + for filename in filenames: + file_info = found.get(filename.replace(' ', '_')) + if file_info is not None: + files.append(file_info) + else: + warnings.append( + 'file "%s" does not exist, please check that its name is spelled correctly, ' + 'that it has not been renamed or removed' % (filename,) + ) + return files, warnings + + def load_by_filename(filenames, source='local'): entries = [] warnings = [] if source == 'remote': files, warnings = get_by_filename_remote(filenames) else: - files = [] - for filename in filenames: - file_info = get_file_info(filename) - if file_info is not None: - files.append(file_info) - else: - warnings.append( - 'file "%s" does not exist, please check that its name is spelled correctly, ' - 'that it has not been renamed or removed' % (filename,) - ) + files, warnings = _lookup_file_names(filenames) for edict in files: entry = make_entry(edict) entries.append(entry) diff --git a/montage/public_endpoints.py b/montage/public_endpoints.py index 128e05a5..8892325b 100644 --- a/montage/public_endpoints.py +++ b/montage/public_endpoints.py @@ -22,7 +22,7 @@ from .mw import public from .rdb import User, PublicDAO -from .labs import get_files, get_file_info +from .labs import get_files, get_files_info_by_names from .utils import get_env_name, DoesNotExist, InvalidAction @@ -145,8 +145,9 @@ def get_files_info_by_name(request_dict: dict[str, Any]) -> dict[str, list[Any]] raise InvalidAction('must provide a list of names') files = [] no_info = [] + found = get_files_info_by_names(file_names) for file_name in file_names: - file_info = get_file_info(file_name) + file_info = found.get(file_name.replace(' ', '_')) if not file_info: no_info.append(file_name) else: diff --git a/montage/rdb.py b/montage/rdb.py index 2bfa56b9..f0d9b840 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -51,7 +51,7 @@ from .imgutils import make_mw_img_url from . import loaders -from .simple_serdes import DictableBase, JSONEncodedDict +from .simple_serdes import DictableBase, JSONEncodedDict, LongJSONEncodedDict Base = declarative_base(cls=DictableBase) @@ -652,7 +652,9 @@ class RoundSource(Base): user_id = Column(Integer, ForeignKey('users.id'), index=True) method = Column(String(255), index=True) - params = Column(JSONEncodedDict) + # MEDIUMTEXT on MySQL: a 'selected' import's file list can pass TEXT's + # 64 KB (tools/migrate_round_sources_params.sql, hatnote/montage#621) + params = Column(LongJSONEncodedDict) dq_params = Column(JSONEncodedDict) create_date = Column(TIMESTAMP, server_default=func.now()) diff --git a/montage/simple_serdes.py b/montage/simple_serdes.py index 9dcc1e50..efeaea6c 100644 --- a/montage/simple_serdes.py +++ b/montage/simple_serdes.py @@ -7,6 +7,7 @@ from sqlalchemy import inspect from sqlalchemy.types import TypeDecorator, Text +from sqlalchemy.dialects.mysql import MEDIUMTEXT from sqlalchemy.ext.mutable import Mutable @@ -123,6 +124,17 @@ def process_result_value(self, value, dialect): return json.loads(value) +class LongJSONEncodedDict(JSONEncodedDict): + """JSONEncodedDict stored as MEDIUMTEXT on MySQL (16 MB instead of + TEXT's 64 KB); plain Text elsewhere. As a JSONEncodedDict subclass it + is MutableDict-associated, so values must be dicts.""" + + def load_dialect_impl(self, dialect): + if dialect.name == 'mysql': + return dialect.type_descriptor(MEDIUMTEXT()) + return dialect.type_descriptor(Text()) + + class MutableDict(Mutable, dict): @classmethod def coerce(cls, key, value): diff --git a/montage/tests/conftest.py b/montage/tests/conftest.py index b0457548..69cc6c4d 100644 --- a/montage/tests/conftest.py +++ b/montage/tests/conftest.py @@ -12,6 +12,7 @@ import json import re +import zlib import pytest import responses as responses_lib @@ -135,6 +136,26 @@ def _disable_pdb(monkeypatch): monkeypatch.setattr('pdb.set_trace', lambda *a, **kw: None) monkeypatch.setattr('pdb.post_mortem', lambda *a, **kw: None) + +# --------------------------------------------------------------------------- +# Wikimedia user-id lookups stay offline. create_app() bootstraps the +# maintainers via get_mw_userid() before any ``responses`` mock is active, so +# without this every app created in a test called the live Commons API, and +# CI started failing once Commons throttled the runner. Slaporte keeps their +# real id because the test session cookie (devtest dev_local_cookie_value, +# base_client) carries userid 6024474. +# --------------------------------------------------------------------------- +KNOWN_MW_USERIDS = {'Slaporte': 6024474} + + +def _offline_mw_userid(username): + return KNOWN_MW_USERIDS.get(username, zlib.crc32(username.encode('utf8')) % 10**8) + + +@pytest.fixture(autouse=True) +def _no_live_mw_userid(monkeypatch): + monkeypatch.setattr('montage.rdb.get_mw_userid', _offline_mw_userid) + # --------------------------------------------------------------------------- # Wikimedia API callback -- returns a plausible user record for any username # --------------------------------------------------------------------------- diff --git a/montage/tests/test_loaders.py b/montage/tests/test_loaders.py index 2817711d..98e6aa96 100644 --- a/montage/tests/test_loaders.py +++ b/montage/tests/test_loaders.py @@ -104,14 +104,336 @@ def test_make_entry_reupload(): assert entry.file_id == 2 +# --------------------------------------------------------------------------- +# Category membership comes from the Commons links replica (x4) since +# 2026-09-08; file data from the main replica. The two are joined in code. +# --------------------------------------------------------------------------- + +class _FakeCursor(object): + def __init__(self, conn): + self.conn = conn + self.rows = None + + def execute(self, query, params): + assert not self.conn.closed + self.conn.queries += 1 + self.rows = self.conn.fetch(query, params, db_host=self.conn.db_host) + + def fetchall(self): + return self.rows + + def close(self): + pass + + +class _FakeConnection(object): + """Stands in for a pymysql connection; each query is answered by a + fetchall_from_commonswiki-style function fetch(query, params, db_host).""" + def __init__(self, db_host, fetch): + self.db_host = db_host + self.fetch = fetch + self.queries = 0 + self.closed = False + + def cursor(self): + return _FakeCursor(self) + + def close(self): + self.closed = True + + +def _patch_replicas(monkeypatch, fetch): + """Replace labs._connect with fake connections answered by fetch; + returns the list of connections opened.""" + from montage import labs + opened = [] + + def connect(db_host): + opened.append(_FakeConnection(db_host, fetch)) + return opened[-1] + monkeypatch.setattr(labs, '_connect', connect) + return opened + + +def _fake_replicas(links_rows, file_rows, calls): + """A replica stand-in: category members from the + links replica, file details from the main replica.""" + from montage import labs + + def fake(query, params, db_host=labs.COMMONS_DB_HOST): + calls.append((db_host, query, params)) + if db_host == labs.COMMONS_LINKS_DB_HOST: + assert 'categorylinks' in query and 'file AS file' not in query + return [{'file_name': n} for n in links_rows] + assert 'categorylinks' not in query and 'linktarget' not in query + return [dict(file_rows[n]) for n in params if n in file_rows] + return fake + + +def _file_row(name, file_id): + return {'img_name': name, 'img_width': 3000, 'img_height': 2000, + 'img_major_mime': 'image', 'img_minor_mime': 'jpeg', + 'img_user': 1, 'img_user_text': 'Uploader', + 'img_timestamp': '20260910120000', + 'rec_img_timestamp': '20260910120000', 'rec_img_user': 1, + 'rec_img_text': 'Uploader', 'oi_archive_name': None, + 'file_id': file_id} + + +def test_get_files_reads_members_from_links_replica(monkeypatch): + from montage import labs + names = ['B_file.jpg', 'A_file.jpg', 'Café_ü.jpg', 'A_file.jpg'] + file_rows = {n: _file_row(n, i) for i, n in enumerate(set(names))} + calls = [] + _patch_replicas(monkeypatch, _fake_replicas(names, file_rows, calls)) + + result = labs.get_files('Images from Wiki Loves Monuments 2026 in Russia') + + assert calls[0][0] == labs.COMMONS_LINKS_DB_HOST + assert calls[0][2] == ('Images_from_Wiki_Loves_Monuments_2026_in_Russia',) + assert all(host == labs.COMMONS_DB_HOST for host, _, _ in calls[1:]) + # one row per file, duplicates collapsed, binary name order as before + assert [r['img_name'] for r in result] == ['A_file.jpg', 'B_file.jpg', 'Café_ü.jpg'] + assert set(result[0]) == set(_file_row('x', 0)) # output keys unchanged + + +def test_get_files_looks_up_files_in_chunks(monkeypatch): + from montage import labs + monkeypatch.setattr(labs, 'FILE_LOOKUP_CHUNK_SIZE', 3) + names = ['F%02d.jpg' % i for i in range(8)] + file_rows = {n: _file_row(n, i) for i, n in enumerate(names)} + calls = [] + _patch_replicas(monkeypatch, _fake_replicas(names, file_rows, calls)) + + result = labs.get_files('Some category') + + lookups = [params for host, _, params in calls if host == labs.COMMONS_DB_HOST] + assert [len(p) for p in lookups] == [3, 3, 2] + assert [r['img_name'] for r in result] == names + + +def test_get_files_skips_members_without_a_file_row(monkeypatch): + """A member with no live file row (deleted) is left out, as the old + inner JOIN did.""" + from montage import labs + names = ['Kept.jpg', 'Deleted.jpg'] + calls = [] + _patch_replicas(monkeypatch, _fake_replicas(names, {'Kept.jpg': _file_row('Kept.jpg', 1)}, calls)) + + assert [r['img_name'] for r in labs.get_files('Cat')] == ['Kept.jpg'] + + +def test_get_files_empty_category_makes_no_file_lookup(monkeypatch): + from montage import labs + calls = [] + _patch_replicas(monkeypatch, _fake_replicas([], {}, calls)) + + assert labs.get_files('Empty category') == [] + assert [host for host, _, _ in calls] == [labs.COMMONS_LINKS_DB_HOST] + + @pytest.mark.xfail( os.environ.get('TOOLFORGE') != '1', - reason='Requires live wikireplica (Toolforge); set TOOLFORGE=1 to run', + reason='Requires live wikireplicas (Toolforge); set TOOLFORGE=1 to run', ) -def test_get_files_parity(): - """New file/filerevision query returns same filenames as old image/oldimage query.""" - from montage.labs import get_files, get_files_legacy - category = 'Images_from_Wiki_Loves_Monuments_2015_in_France' - new = {r['img_name'] for r in get_files(category)} - old = {r['img_name'] for r in get_files_legacy(category)} - assert new == old +def test_get_files_sees_files_added_after_links_split(): + """On 2026-10-01 the stale main-replica copy of categorylinks had 5,005 + links for this category and the links replica 15,678. get_files must + follow the links replica.""" + from montage.labs import get_category_file_names, get_files + category = 'Images_from_Wiki_Loves_Monuments_2026_in_Russia' + members = get_category_file_names(category) + files = get_files(category) + assert len(members) > 10000 + assert len(files) >= 0.99 * len(set(members)) + + +# --------------------------------------------------------------------------- +# File-list imports look names up in batches, not one query per file +# (a 1000+ name list outlasted the 30 s request timeout on montage-beta). +# --------------------------------------------------------------------------- + +def _fake_main_replica(existing, calls): + from montage import labs + + def fake(query, params, db_host=labs.COMMONS_DB_HOST): + assert db_host == labs.COMMONS_DB_HOST + calls.append(params) + return [_file_row(n, i) for i, n in enumerate(params) if n in existing] + return fake + + +def test_load_by_filename_batches_lookups(monkeypatch): + from montage import labs, loaders + monkeypatch.setattr(labs, 'FILE_LOOKUP_CHUNK_SIZE', 4) + names = ['File %02d.jpg' % i for i in range(10)] + existing = {n.replace(' ', '_') for n in names} + calls = [] + _patch_replicas(monkeypatch, _fake_main_replica(existing, calls)) + + entries, warnings = loaders.load_by_filename(names, source='local') + + assert [len(c) for c in calls] == [4, 4, 2] # 3 queries, not 10 + assert [e.name for e in entries] == [n.replace(' ', '_') for n in names] + assert warnings == [] + + +def test_load_by_filename_warns_per_missing_name_in_input_order(monkeypatch): + from montage import labs, loaders + calls = [] + _patch_replicas(monkeypatch, _fake_main_replica({'B.jpg'}, calls)) + + entries, warnings = loaders.load_by_filename(['Z missing.jpg', 'B.jpg', 'A missing.jpg'], + source='local') + + assert [e.name for e in entries] == ['B.jpg'] + assert len(warnings) == 2 + assert '"Z missing.jpg"' in warnings[0] and '"A missing.jpg"' in warnings[1] + + +def test_load_name_list_local(monkeypatch): + """CSV with only file names, local source: used to unpack the file info + dict into (edict, warnings) and fail.""" + from io import StringIO + from montage import labs, loaders + calls = [] + _patch_replicas(monkeypatch, _fake_main_replica({'One.jpg', 'Two_words.jpg'}, calls)) + + entries, warnings = loaders.load_name_list( + StringIO('File:One.jpg\nTwo words.jpg\nGone.jpg\n'), source='local') + + assert [e.name for e in entries] == ['One.jpg', 'Two_words.jpg'] + assert len(calls) == 1 + assert len(warnings) == 1 and '"Gone.jpg"' in warnings[0] + + +def test_round_source_params_hold_large_file_lists(): + """A 'selected' import stores its whole file list in round_sources.params; + 1000+ names pass TEXT's 64 KB, so the column is MEDIUMTEXT on MySQL.""" + from sqlalchemy import create_engine + from sqlalchemy.dialects import mysql + from sqlalchemy.orm import sessionmaker + from sqlalchemy.schema import CreateTable + from montage.rdb import Base, RoundSource + + ddl = str(CreateTable(RoundSource.__table__).compile(dialect=mysql.dialect())) + assert 'params MEDIUMTEXT' in ddl + + engine = create_engine('sqlite://') + Base.metadata.create_all(engine) + session = sessionmaker(bind=engine)() + names = ['Wiki_Loves_Monuments_2026_in_Russia_photo_%05d.jpg' % i for i in range(2000)] + session.add(RoundSource(method='selected', params={'file_names': names})) + session.commit() + stored = session.query(RoundSource).one().params + assert stored['file_names'] == names + assert len(str(stored)) > 64 * 1024 + + +def test_get_files_order_is_binary_like_the_old_query(monkeypatch): + """Uppercase sorts before lowercase, as the old ORDER BY on the binary + file_name column did; a case-insensitive sort would differ.""" + from montage import labs + names = ['b_lower.jpg', 'B_upper.jpg', 'a_lower.jpg', 'A_upper.jpg', 'Ö_umlaut.jpg'] + calls = [] + _patch_replicas(monkeypatch, _fake_replicas(names, {n: _file_row(n, i) for i, n in enumerate(names)}, calls)) + + result = [r['img_name'] for r in labs.get_files('Cat')] + + assert result == ['A_upper.jpg', 'B_upper.jpg', 'a_lower.jpg', 'b_lower.jpg', 'Ö_umlaut.jpg'] + + +def test_load_by_filename_duplicates_and_space_underscore_pairs(monkeypatch): + """'A b.jpg' and 'A_b.jpg' are the same Commons file; both requests + resolve, with one query.""" + from montage import labs, loaders + calls = [] + _patch_replicas(monkeypatch, _fake_main_replica({'A_b.jpg'}, calls)) + + entries, warnings = loaders.load_by_filename(['A b.jpg', 'A_b.jpg', 'A b.jpg'], source='local') + + assert [e.name for e in entries] == ['A_b.jpg'] * 3 + assert warnings == [] and len(calls) == 1 and calls[0] == ('A_b.jpg',) + + +# --------------------------------------------------------------------------- +# One replica connection per lookup (per database host), closed afterwards, +# not one per 500-name chunk (hatnote/montage#625). +# --------------------------------------------------------------------------- + +def test_get_files_uses_one_connection_per_replica(monkeypatch): + from montage import labs + monkeypatch.setattr(labs, 'FILE_LOOKUP_CHUNK_SIZE', 3) + names = ['F%02d.jpg' % i for i in range(8)] + calls = [] + opened = _patch_replicas(monkeypatch, _fake_replicas( + names, {n: _file_row(n, i) for i, n in enumerate(names)}, calls)) + + assert len(labs.get_files('Some category')) == 8 + + # links + main, although the main replica answered 3 chunk queries + assert [c.db_host for c in opened] == [labs.COMMONS_LINKS_DB_HOST, + labs.COMMONS_DB_HOST] + assert [c.queries for c in opened] == [1, 3] + assert all(c.closed for c in opened) + + +def test_get_files_info_by_names_uses_one_connection(monkeypatch): + from montage import labs + monkeypatch.setattr(labs, 'FILE_LOOKUP_CHUNK_SIZE', 4) + names = ['File %02d.jpg' % i for i in range(10)] + calls = [] + opened = _patch_replicas(monkeypatch, _fake_main_replica( + {n.replace(' ', '_') for n in names}, calls)) + + assert len(labs.get_files_info_by_names(names)) == 10 + + assert [len(c) for c in calls] == [4, 4, 2] + assert [(c.db_host, c.queries, c.closed) for c in opened] == [ + (labs.COMMONS_DB_HOST, 3, True)] + + +def test_replica_connection_is_closed_when_a_query_fails(monkeypatch): + from montage import labs + + def failing(query, params, db_host=labs.COMMONS_DB_HOST): + raise RuntimeError('replica went away') + opened = _patch_replicas(monkeypatch, failing) + + with pytest.raises(RuntimeError): + labs.get_files_info_by_names(['A.jpg']) + with pytest.raises(RuntimeError): + labs.fetchall_from_commonswiki('SELECT 1', ()) + + assert len(opened) == 2 and all(c.closed for c in opened) + + +def _labs_hosts(env): + """(COMMONS_DB_HOST, COMMONS_LINKS_DB_HOST) as a fresh interpreter + computes them at import, with these MONTAGE_COMMONS_* variables.""" + import subprocess + import sys + run_env = dict(os.environ) + run_env.pop('MONTAGE_COMMONS_DB_HOST', None) + run_env.pop('MONTAGE_COMMONS_LINKS_DB_HOST', None) + run_env.update(env) + root = os.path.dirname(os.path.dirname(os.path.dirname( + os.path.abspath(__file__)))) + out = subprocess.check_output( + [sys.executable, '-c', + 'import montage.labs as l; print(l.COMMONS_DB_HOST); ' + 'print(l.COMMONS_LINKS_DB_HOST)'], + env=run_env, cwd=root) + return tuple(out.decode('utf8').split()) + + +def test_replica_hosts_default_to_the_analytics_replicas(): + assert _labs_hosts({}) == ( + 'commonswiki.analytics.db.svc.wikimedia.cloud', + 'links.commonswiki.analytics.db.svc.wikimedia.cloud') + + +def test_replica_hosts_can_be_set_by_environment(): + assert _labs_hosts({'MONTAGE_COMMONS_DB_HOST': 'main.example', + 'MONTAGE_COMMONS_LINKS_DB_HOST': 'links.example'}) == ( + 'main.example', 'links.example') diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index 44605851..7d2e5147 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -867,7 +867,8 @@ def test_multiple_jurors(api_client, mock_external_apis): def test_get_files_info_by_name(api_client): """GET /utils/file returns file_infos with file_id populated.""" from .conftest import SELECTED_FILE_INFO - with patch('montage.public_endpoints.get_file_info', return_value=SELECTED_FILE_INFO): + with patch('montage.public_endpoints.get_files_info_by_names', + return_value={SELECTED_FILE_INFO['img_name'].replace(' ', '_'): SELECTED_FILE_INFO}): resp = api_client.fetch( 'public: get file info by name', '/utils/file', @@ -1234,3 +1235,92 @@ def test_oauth_complete_login_success(oauth_app): data = _get_cookie_data(client, oauth_app.resources['config']['cookie_secret']) assert data.get('userid') == 12345 assert data.get('username') == 'OAuthTestUser' + + +def _import_result(montage_app, api_client, round_id, data): + """The result of an /admin/round//import call, in either mode. + + Synchronous import: the response data itself. Background import worker + (hatnote/montage#621): the response carries data['job']; run the queue + and return the job's data, which holds the warnings. On branches + without the worker the second path is never taken. + """ + if 'job' not in data: + return data + from montage.tests.conftest import run_import_jobs + job_id = data['job']['id'] + assert run_import_jobs(montage_app) == [(job_id, 'succeeded')] + return api_client.fetch('coordinator: import job', + '/admin/round/%s/import/%s' % (round_id, job_id), + as_user='Yarl')['data'] + + +def test_selected_import_with_missing_name_reports_warning(montage_app, api_client, mock_external_apis): + """A file-list import where some names are not found must succeed with a + warning. The warning used to be a set, which can't be serialised (500).""" + import responses as responses_lib + from montage.tests.conftest import TOOLFORGE_FILE_URL, SELECTED_FILE_INFO + + mock_external_apis.replace(responses_lib.POST, TOOLFORGE_FILE_URL, + json={'file_infos': [SELECTED_FILE_INFO], + 'no_info': ['Not_on_Commons.jpg']}, + status=200) + api_client.fetch('maintainer: add organizer', '/admin/add_organizer', + {'username': 'Yarl'}) + series_id = api_client.fetch('get default series', '/series')['data'][0]['id'] + campaign_id = api_client.fetch( + 'organizer: create campaign', '/admin/add_campaign', + {'name': 'missing name test', 'coordinators': ['Yarl'], + 'open_date': '2014-01-01T00:00:00', 'close_date': '2016-01-01T00:00:00', + 'url': 'http://hatnote.com', 'series_id': series_id}, + as_user='Yarl')['data']['id'] + round_id = api_client.fetch( + 'coordinator: create round', '/admin/campaign/%s/add_round' % campaign_id, + {'name': 'r', 'vote_method': 'yesno', 'deadline_date': '2016-10-15T00:00:00', + 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']}, + as_user='Yarl')['data']['id'] + + data = api_client.fetch( + 'coordinator: import selected files', '/admin/round/%s/import' % round_id, + {'import_method': 'selected', + 'file_names': [SELECTED_FILE_INFO['img_name'], 'Not on Commons.jpg']}, + as_user='Yarl')['data'] + data = _import_result(montage_app, api_client, round_id, data) + + issues = [w for w in data['warnings'] if 'import issues' in w] + assert len(issues) == 1 and 'Not_on_Commons.jpg' in issues[0]['import issues'] + + +def test_csv_import_warnings_are_dicts(montage_app, api_client, mock_external_apis, monkeypatch): + """The frontend shows one value per warning; a plain string showed only + its last character. CSV warnings must be dicts like the others.""" + from montage import loaders + from montage.tests.conftest import FIXTURE_FILE_INFOS + + def fake_csv(url, source='local'): + entries = [loaders.make_entry(info) for info in FIXTURE_FILE_INFOS[:3]] + return entries, ['file "Gone.jpg" does not exist'] + monkeypatch.setattr(loaders, 'get_entries_from_csv', fake_csv) + + api_client.fetch('maintainer: add organizer', '/admin/add_organizer', {'username': 'Yarl'}) + series_id = api_client.fetch('get default series', '/series')['data'][0]['id'] + campaign_id = api_client.fetch( + 'organizer: create campaign', '/admin/add_campaign', + {'name': 'csv warning test', 'coordinators': ['Yarl'], + 'open_date': '2014-01-01T00:00:00', 'close_date': '2016-01-01T00:00:00', + 'url': 'http://hatnote.com', 'series_id': series_id}, + as_user='Yarl')['data']['id'] + round_id = api_client.fetch( + 'coordinator: create round', '/admin/campaign/%s/add_round' % campaign_id, + {'name': 'r', 'vote_method': 'yesno', 'deadline_date': '2016-10-15T00:00:00', + 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']}, + as_user='Yarl')['data']['id'] + + data = api_client.fetch( + 'coordinator: import csv', '/admin/round/%s/import' % round_id, + {'import_method': 'csv', 'csv_url': 'https://example.org/files.csv'}, + as_user='Yarl')['data'] + data = _import_result(montage_app, api_client, round_id, data) + + assert data['warnings'] and all(isinstance(w, dict) for w in data['warnings']) + assert any('Gone.jpg' in w.get('import issues', '') for w in data['warnings']) diff --git a/tools/migrate_round_sources_params.sql b/tools/migrate_round_sources_params.sql new file mode 100644 index 00000000..35a4c8e8 --- /dev/null +++ b/tools/migrate_round_sources_params.sql @@ -0,0 +1,17 @@ +-- Migration: widen round_sources.params from TEXT (64 KB) to MEDIUMTEXT (16 MB). +-- Companion to revert_round_sources_params.sql. +-- Idempotent: safe to run more than once (MODIFY to the same type is a no-op). +-- +-- Why: a 'selected' import stores its whole file list in round_sources.params; +-- a few thousand long file names pass 64 KB, which strict mode rejects (the +-- import then fails). The model uses LongJSONEncodedDict for this column. +-- Part of hatnote/montage#621 (background import job, Phase 2). +-- +-- Order: any. The schema check (montage/check_rdb.py) does not compare column +-- types, so code with or without this change runs on either column type. +-- Run from Toolforge as the tool account, e.g.: +-- mariadb --defaults-file=~/replica.my.cnf -h tools.db.svc.wikimedia.cloud < tools/migrate_round_sources_params.sql +-- On a large round_sources table the ALTER copies the table; run it when no +-- import is running. + +ALTER TABLE round_sources MODIFY params MEDIUMTEXT NULL; diff --git a/tools/revert_round_sources_params.sql b/tools/revert_round_sources_params.sql new file mode 100644 index 00000000..0be6d61e --- /dev/null +++ b/tools/revert_round_sources_params.sql @@ -0,0 +1,9 @@ +-- Revert: round_sources.params back to TEXT. Reverse of +-- migrate_round_sources_params.sql. +-- +-- CAUTION: rows whose params are longer than 65,535 bytes make this fail +-- (strict mode) or get truncated, which corrupts their JSON. Check first: +-- SELECT id, LENGTH(params) FROM round_sources WHERE LENGTH(params) > 65535; +-- Part of hatnote/montage#621 (background import job, Phase 2). + +ALTER TABLE round_sources MODIFY params TEXT NULL;