From 56fdf604a9bb1b6386ec049b43b338ad9ecffbe5 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:21:54 +0000 Subject: [PATCH 01/20] wip(#510): import errors, labs lookup by file_id, source fetch/parse - utils: ImportSourceInvalid/CheckBlocked/CheckExpired/CheckRequired (400 with error_type); config key import_check_path (MONTAGE_IMPORT_CHECK_PATH) - labs: one query text for lookups by name and by file_id; get_files_info_by_ids - loaders: fetch_source_text, parse_source_rows (filename > img_name, optional file_id, BOM/case-insensitive headers), parse_name_list, lookups; old full-CSV/name-list loaders removed (#509, #517); only gist links are rewritten (#208) Refs hatnote/montage#510, #509, #208 Co-Authored-By: Claude Opus 5.5 --- montage/labs.py | 43 ++++- montage/loaders.py | 349 ++++++++++++++++++++-------------- montage/tests/test_loaders.py | 327 +++++++++++++++++++++---------- montage/utils.py | 33 ++++ 4 files changed, 500 insertions(+), 252 deletions(-) diff --git a/montage/labs.py b/montage/labs.py index dd6e6e18..c7223b91 100644 --- a/montage/labs.py +++ b/montage/labs.py @@ -150,10 +150,11 @@ def get_files_by_name(file_names): return _files_by_name(fetchall, file_names) -def _files_by_name(fetchall, file_names): - if not file_names: - return [] - query = ''' +def _files_query(column, count): + """The file details query for `count` values of file.; one + query text for lookups by name and by file_id.""" + assert column in ('file_name', 'file_id') + return ''' SELECT DISTINCT {cols} FROM commonswiki_p.file AS file JOIN commonswiki_p.filerevision AS fr ON fr.fr_id = file.file_latest @@ -161,14 +162,28 @@ def _files_by_name(fetchall, file_names): 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} - WHERE file.file_name IN ({names}) + WHERE file.{column} IN ({values}) AND file.file_deleted = 0 '''.format(cols=', '.join(FILE_COLS), earliest_rev=_EARLIEST_REVISION_SUBQUERY, - names=', '.join(['%s'] * len(file_names))) + column=column, + values=', '.join(['%s'] * count)) + + +def _files_by_name(fetchall, file_names): + if not file_names: + return [] + query = _files_query('file_name', len(file_names)) return fetchall(query, tuple(file_names)) +def _files_by_id(fetchall, file_ids): + if not file_ids: + return [] + query = _files_query('file_id', len(file_ids)) + return fetchall(query, tuple(file_ids)) + + 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.""" @@ -184,6 +199,22 @@ def get_files_info_by_names(file_names): return ret +def get_files_info_by_ids(file_ids): + """{file_id: file info} for the file_ids of live files (Commons' + file.file_id), in chunks on one connection, like + get_files_info_by_names (hatnote/montage#510).""" + ids = sorted(set(int(i) for i in file_ids)) + ret = {} + if not ids: + return ret + with commonswiki_connection(COMMONS_DB_HOST) as fetchall: + for i in range(0, len(ids), FILE_LOOKUP_CHUNK_SIZE): + chunk = ids[i:i + FILE_LOOKUP_CHUNK_SIZE] + for rec in _files_by_id(fetchall, chunk): + ret[int(rec['file_id'])] = rec + return ret + + 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. diff --git a/montage/loaders.py b/montage/loaders.py index 8a53b561..bdcfcf8d 100644 --- a/montage/loaders.py +++ b/montage/loaders.py @@ -2,29 +2,25 @@ from __future__ import absolute_import import datetime -from io import BytesIO, StringIO +from io import StringIO +import csv import json import re +import requests from boltons.iterutils import chunked_iter -from unicodecsv import DictReader import montage.rdb # TODO: circular import -from .labs import get_files, get_files_info_by_names -from .utils import unicode, requests_get, requests_post +from .labs import get_files, get_files_info_by_names, get_files_info_by_ids +from .utils import (ImportSourceInvalid, to_unicode, requests_get, + requests_post) REMOTE_UTILS_URL = 'https://montage.toolforge.org/v1/utils/' GSHEET_URL = 'https://docs.google.com/spreadsheets/d/%s/gviz/tq?tqx=out:csv' -CSV_FULL_COLS = ['img_name', - 'img_major_mime', - 'img_minor_mime', - 'img_width', - 'img_height', - 'img_user', - 'img_user_text', - 'img_timestamp'] +# seconds; a source link is fetched within the 30 s request limit +SOURCE_FETCH_TIMEOUT = 15 def wpts2dt(timestamp): @@ -68,8 +64,8 @@ def make_entry(edict): 'archive_name': edict['oi_archive_name']} raw_entry['upload_date'] = wpts2dt(edict['img_timestamp']) raw_entry['resolution'] = width * height - # file_id is only available from wikireplica imports (labs.py), not CSV. - # CSV-imported entries intentionally get file_id=NULL. + # file_id comes from Commons (labs.py); every checked import has one, + # except dev's remote lookups where the utils endpoint returns none. if edict.get('file_id') is not None: raw_entry['file_id'] = edict['file_id'] if edict.get('flags'): @@ -77,165 +73,222 @@ def make_entry(edict): return montage.rdb.Entry(**raw_entry) -def load_full_csv(csv_file_obj, source='remote'): - # TODO: streaming this for big CSVs is an unnecessary headache +# --------------------------------------------------------------------------- +# Import sources (hatnote/montage#510): every first-round source is fetched +# and parsed into rows here, then looked up on Commons and classified by +# import_check.py. Commons supplies all metadata; a CSV only gives names and, +# optionally, Commons file_ids (#509). +# --------------------------------------------------------------------------- - ret = [] - warnings = [] +NAME_COLUMNS = ('filename', 'img_name') # in order of preference +FILE_ID_COLUMN = 'file_id' - dr = DictReader(csv_file_obj) +# a header-less list's first line is a file name, not a header, when it ends +# in a file extension (header words never do) +_FILE_EXT_RE = re.compile(r'\.[A-Za-z0-9]{2,5}$') +_FILE_ID_RE = re.compile(r'^[0-9]+$') - if 'filename' in dr.fieldnames: - return load_partial_csv(dr, source=source) +# Cells starting with one of these get a leading ' in Montage's CSV +# downloads, so spreadsheet programs do not run them as formulas; reading +# a CSV removes that ' again, so a download can be uploaded unchanged. +FORMULA_START_CHARS = "=+-@'" - for key in CSV_FULL_COLS: - if key not in dr.fieldnames: - raise ValueError('missing required column "%s" in csv file' % key) - for edict in dr: - try: - entry = make_entry(edict) - except TypeError as e: - warnings.append((edict, e)) - else: - ret.append(entry) +def guard_cell(value): + if value and value[0] in FORMULA_START_CHARS: + return "'" + value + return value - return ret, warnings +def unguard_cell(value): + if len(value) > 1 and value[0] == "'" and value[1] in FORMULA_START_CHARS: + return value[1:] + return value -def load_partial_csv(dr, source='remote'): - ret = [] - warnings = [] - file_names = [r['filename'] for r in dr] - if not file_names or isinstance(file_names[0], unicode): - file_names_obj = StringIO('\n'.join(file_names)) - else: - file_names_obj = BytesIO(b'\n'.join(file_names)) - return load_name_list(file_names_obj, source=source) +def normalise_name(name): + """A file name as Montage and the replica store it: no File: prefix, + underscores for spaces.""" + name = unguard_cell((name or '').strip()) + if name.startswith('File:'): + name = name[5:].strip() + return name.replace(' ', '_') -def load_name_list(file_obj, source='local'): - """ Just the file names, and we'll look up the rest""" +def parse_file_id(cell): + """(file_id, malformed) for a file_id cell: a plain whole number above + 0, or empty. 1.49673E+08, 149,673,020 or 12.0 are malformed.""" + value = unguard_cell((cell or '').strip().strip(u'\xa0').strip()) + if not value: + return None, False + if _FILE_ID_RE.match(value) and int(value) > 0: + return int(value), False + return None, True - ret = [] - warnings = [] - rl = file_obj.readlines() +def _source_row(row_number, name_cell, file_id_cell=''): + file_id, malformed = parse_file_id(file_id_cell) + return {'row': row_number, + 'name_as_written': name_cell or '', + 'file_id_as_written': file_id_cell or '', + 'name': normalise_name(name_cell), + 'file_id': file_id, + 'malformed_file_id': malformed} - # clean up the filenames - for i, filename in enumerate(rl): - if isinstance(filename, bytes): - filename = filename.decode('utf8') - filename = filename.strip() - if filename.startswith('File:'): - filename = filename[5:] - rl[i] = filename - edicts = [] +def _is_blank(row): + return not row['name'] and not row['file_id_as_written'].strip() - if source == 'remote': - edicts, warnings = get_by_filename_remote(rl) - else: - edicts, warnings = _lookup_file_names(rl) - for edict in edicts: - try: - entry = make_entry(edict) - except TypeError as e: - warnings.append((edict, e)) - else: - ret.append(entry) +def _header_key(cell): + return cell.replace(u'\ufeff', '').strip().lower() - return ret, warnings -def get_entries_from_csv(raw_url, source='local'): - if 'google.com' in raw_url: - return get_entries_from_gsheet(raw_url, source) - return get_entries_from_gist(raw_url, source) +def _looks_like_file_name(line): + value = line.strip().strip('"').strip() + if value.startswith('File:'): + value = value[5:] + return bool(_FILE_EXT_RE.search(value)) -def get_entries_from_gist(raw_url, source='local'): - if 'githubusercontent' not in raw_url: - raw_url = raw_url.replace('gist.github.com', - 'gist.githubusercontent.com') + '/raw' - resp = requests_get(raw_url) +def _line_name(line): + """A header-less list's line: one CSV cell (a quoted name, as Google + Sheets exports), or else the whole line, so commas in names survive.""" try: - ret, warnings = load_full_csv(BytesIO(resp.content)) - except ValueError as e: - # not a full csv - ret, warnings = load_name_list(BytesIO(resp.content), source=source) - - return ret, warnings - - -def get_entries_from_gsheet(raw_url, source='local'): - #TODO: add support for sheet tabs - doc_id = parse_doc_id(raw_url) - url = GSHEET_URL % doc_id - resp = requests_get(url) - - if not 'text/csv' in resp.headers['content-type']: - raise ValueError('cannot load Google Sheet "%s" (is link sharing on?)' % raw_url) - + cells = next(csv.reader([line])) + except (StopIteration, csv.Error): + cells = [] + if len(cells) == 1: + return cells[0] + return line.strip().strip('"') + + +def parse_source_rows(text): + """(rows, columns) of a CSV or a header-less name list. + + The name column is `filename`, else `img_name`; `file_id` is optional; + headers are matched case-insensitively, ignoring a BOM. Other columns + are ignored (Commons supplies the metadata) and listed in + columns['ignored']. Row numbers are spreadsheet rows (header = 1) or, + for a header-less list, line numbers; blank rows are skipped. + """ + columns = {'name': None, 'file_id': None, 'ignored': []} try: - ret, warnings = load_full_csv(BytesIO(resp.content), source=source) - except ValueError: + records = list(csv.reader(StringIO(text))) + except csv.Error as e: + raise ImportSourceInvalid('cannot read the CSV: %s' % (e,)) + if not records: + return [], columns + header = [_header_key(c) for c in records[0]] + name_col = next((c for c in NAME_COLUMNS if c in header), None) + rows = [] + if name_col is not None: + name_idx = header.index(name_col) + id_idx = header.index(FILE_ID_COLUMN) if FILE_ID_COLUMN in header else None + columns['name'] = name_col + columns['file_id'] = FILE_ID_COLUMN if id_idx is not None else None + columns['ignored'] = [c.replace(u'\ufeff', '').strip() + for i, c in enumerate(records[0]) + if i not in (name_idx, id_idx) and c.strip()] + for i, record in enumerate(records[1:]): + name = record[name_idx] if name_idx < len(record) else '' + file_id = (record[id_idx] if id_idx is not None + and id_idx < len(record) else '') + rows.append(_source_row(i + 2, name, file_id)) + else: + lines = text.splitlines() + if len(records[0]) > 1 and not _looks_like_file_name(lines[0]): + raise ImportSourceInvalid( + 'no filename or img_name column: the first row has %s columns' + ' but none is called "filename" or "img_name"' + % len(records[0])) + for i, line in enumerate(lines): + rows.append(_source_row(i + 1, _line_name(line))) + return [r for r in rows if not _is_blank(r)], columns + + +def parse_name_list(file_names): + """Rows of a file list (import method `selected`): a list of names or + one name per line; blank entries are skipped but keep their number.""" + if file_names is None: + raise ImportSourceInvalid('no file names given') + if isinstance(file_names, (str, bytes)): + file_names = to_unicode(file_names).splitlines() + rows = [_source_row(i + 1, to_unicode(name or '')) + for i, name in enumerate(file_names)] + return [r for r in rows if not _is_blank(r)] + + +def source_url(raw_url): + """(fetch url, is a Google Sheet) for a gist / CSV / Sheet link.""" + if 'google.com' in raw_url: try: - ret, warnings = load_partial_csv(resp) # TODO: load_partial_csv expects a dictreader, did this ever work? - except (ValueError, TypeError): - resp_content = resp.content.decode('utf8') - file_names = [fn.strip('\"') for fn in resp_content.split('\n')] - file_names_obj = StringIO('\n'.join(file_names)) - ret, warnings = load_name_list(file_names_obj, source=source) + doc_id = parse_doc_id(raw_url) + except ValueError as e: + raise ImportSourceInvalid(str(e)) + return GSHEET_URL % doc_id, True + if 'gist.github.com' in raw_url and 'githubusercontent' not in raw_url: + # a gist's page -> its raw text; other links are fetched as given + # (#208: '/raw' used to be appended to every link) + raw_url = raw_url.replace('gist.github.com', + 'gist.githubusercontent.com') + '/raw' + return raw_url, False - return ret, warnings +def fetch_source_text(raw_url): + """The text of a gist / CSV link or a Google Sheet's CSV export. No + Commons lookup happens here.""" + raw_url = (raw_url or '').strip() + if not raw_url: + raise ImportSourceInvalid('no link given') + url, is_sheet = source_url(raw_url) + try: + resp = requests_get(url, timeout=SOURCE_FETCH_TIMEOUT) + except requests.RequestException as e: + raise ImportSourceInvalid('cannot load "%s" (%s)' + % (raw_url, e.__class__.__name__)) + if resp.status_code != 200: + raise ImportSourceInvalid('cannot load "%s" (HTTP status %s)' + % (raw_url, resp.status_code)) + if is_sheet and 'text/csv' not in resp.headers.get('content-type', ''): + raise ImportSourceInvalid('cannot load Google Sheet "%s" (is link sharing on?)' + % raw_url) + try: + return resp.content.decode('utf-8-sig') + except UnicodeDecodeError: + raise ImportSourceInvalid('"%s" is not UTF-8 text' % raw_url) -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 = [] +def lookup_by_names(names, source='local'): + """{normalised name: Commons file info} for the names that exist.""" + names = sorted(set(n for n in names if n)) + if not names: + return {} if source == 'remote': - files, warnings = get_by_filename_remote(filenames) - else: - files, warnings = _lookup_file_names(filenames) - for edict in files: - entry = make_entry(edict) - entries.append(entry) - return entries, warnings - - -def load_category(category_name, source='local'): - ret = [] + file_infos, _ = get_by_filename_remote(names) + wanted = set(names) + found = {} + for rec in file_infos: + name = normalise_name(rec['img_name']) + if name in wanted: # infos for names not asked for are ignored + found[name] = rec + return found + return get_files_info_by_names(names) + + +def lookup_by_ids(file_ids): + """{file_id: Commons file info} for the file_ids of live files; on the + wikireplica only (the public utils endpoint has no lookup by id).""" + return get_files_info_by_ids(file_ids) + + +def category_records(category_name, source='local'): + """Commons file infos of a category's files.""" + if not (category_name or '').strip(): + raise ImportSourceInvalid('no category given') if source == 'remote': - files = get_from_category_remote(category_name) - else: - files = get_files(category_name) - for edict in files: - entry = make_entry(edict) - ret.append(entry) - - return ret + return get_from_category_remote(category_name) + return get_files(category_name) def get_from_category_remote(category_name): @@ -247,9 +300,13 @@ def get_from_category_remote(category_name): def get_from_remote(url, params): headers = {'Content-Type': 'application/json', 'Accept': 'application/json'} data = json.dumps(params) - response = requests_post(url, data=data, headers=headers) - resp_json = response.json() - file_infos = resp_json['file_infos'] + try: + response = requests_post(url, data=data, headers=headers) + resp_json = response.json() + file_infos = resp_json['file_infos'] + except (requests.RequestException, ValueError, KeyError, TypeError) as e: + raise ImportSourceInvalid('Commons lookup through %s failed (%s)' + % (url, e.__class__.__name__)) no_infos = resp_json.get('no_info') return file_infos, no_infos diff --git a/montage/tests/test_loaders.py b/montage/tests/test_loaders.py index 98e6aa96..6f97e4e8 100644 --- a/montage/tests/test_loaders.py +++ b/montage/tests/test_loaders.py @@ -9,7 +9,10 @@ import responses from pytest import raises -from montage.loaders import get_entries_from_gsheet, make_entry +from montage.loaders import (make_entry, parse_source_rows, parse_name_list, + parse_file_id, normalise_name, fetch_source_text, + guard_cell, unguard_cell) +from montage.utils import ImportSourceInvalid from .conftest import ( FIXTURE_FILE_INFOS, @@ -20,66 +23,24 @@ ) RESULTS = 'https://docs.google.com/spreadsheets/d/1RDlpT23SV_JB1mIz0OA-iuc3MNdNVLbaK_LtWAC7vzg/edit?usp=sharing' -FILENAME_LIST = 'https://docs.google.com/spreadsheets/d/1Nqj-JsX3L5qLp5ITTAcAFYouglbs5OpnFwP6zSFpa0M/edit?usp=sharing' -GENERIC_CSV = 'https://docs.google.com/spreadsheets/d/1WzHFg_bhvNthRMwNmxnk010KJ8fwuyCrby29MvHUzH8/edit#gid=550467819' FORBIDDEN_SHEET = 'https://docs.google.com/spreadsheets/d/1tza92brMKkZBTykw3iS6X9ij1D4_kvIYAiUlq1Yi7Fs/edit' # Pre-compute the actual fetch URLs that loaders.py constructs from the # doc IDs embedded in the spreadsheet URLs above. _RESULTS_CSV_URL = 'https://docs.google.com/spreadsheets/d/1RDlpT23SV_JB1mIz0OA-iuc3MNdNVLbaK_LtWAC7vzg/gviz/tq?tqx=out:csv' -_FILENAME_CSV_URL = 'https://docs.google.com/spreadsheets/d/1Nqj-JsX3L5qLp5ITTAcAFYouglbs5OpnFwP6zSFpa0M/gviz/tq?tqx=out:csv' -_GENERIC_CSV_URL = 'https://docs.google.com/spreadsheets/d/1WzHFg_bhvNthRMwNmxnk010KJ8fwuyCrby29MvHUzH8/gviz/tq?tqx=out:csv' _FORBIDDEN_CSV_URL = 'https://docs.google.com/spreadsheets/d/1tza92brMKkZBTykw3iS6X9ij1D4_kvIYAiUlq1Yi7Fs/gviz/tq?tqx=out:csv' -@responses.activate -def test_load_results(): - """Full CSV with all required columns -- entries created directly.""" - responses.add( - responses.GET, - _RESULTS_CSV_URL, - body=FIXTURE_FULL_CSV, - status=200, - content_type='text/csv', - ) - imgs, warnings = get_entries_from_gsheet(RESULTS, source='remote') - assert len(imgs) == len(FIXTURE_FILE_INFOS) - - -@responses.activate -def test_load_filenames(): - """Partial CSV with only 'filename' column -- triggers Toolforge lookup.""" - responses.add( - responses.GET, - _FILENAME_CSV_URL, - body=FIXTURE_FILENAME_CSV, - status=200, - content_type='text/csv', - ) - # The filename-only CSV triggers load_partial_csv -> load_name_list - # -> get_by_filename_remote -> POST to Toolforge /file endpoint. - responses.add( - responses.POST, - TOOLFORGE_FILE_URL, - json={'file_infos': FIXTURE_FILE_INFOS, 'no_info': []}, - status=200, - ) - imgs, warnings = get_entries_from_gsheet(FILENAME_LIST, source='remote') - assert len(imgs) == len(FIXTURE_FILE_INFOS) - +# --------------------------------------------------------------------------- +# Fetching a source (hatnote/montage#510): no Commons lookup here +# --------------------------------------------------------------------------- @responses.activate -def test_load_csv(): - """Generic full CSV -- same path as test_load_results.""" - responses.add( - responses.GET, - _GENERIC_CSV_URL, - body=FIXTURE_FULL_CSV, - status=200, - content_type='text/csv', - ) - imgs, warnings = get_entries_from_gsheet(GENERIC_CSV, source='remote') - assert len(imgs) == len(FIXTURE_FILE_INFOS) +def test_fetch_sheet_returns_its_csv_text(): + responses.add(responses.GET, _RESULTS_CSV_URL, body=FIXTURE_FULL_CSV, + status=200, content_type='text/csv') + assert fetch_source_text(RESULTS) == FIXTURE_FULL_CSV + assert len(responses.calls) == 1 @responses.activate @@ -92,8 +53,118 @@ def test_no_persmission(): status=200, content_type='text/html', ) - with raises(ValueError): - get_entries_from_gsheet(FORBIDDEN_SHEET, source='remote') + with raises(ImportSourceInvalid): + fetch_source_text(FORBIDDEN_SHEET) + + +@responses.activate +def test_fetch_gist_rewrites_the_link_and_strips_the_bom(): + responses.add(responses.GET, 'https://gist.githubusercontent.com/u/abc/raw', + body=u'\ufefffilename\nA.jpg\n'.encode('utf8'), status=200) + assert fetch_source_text('https://gist.github.com/u/abc') == 'filename\nA.jpg\n' + + +@responses.activate +def test_fetch_errors_are_import_source_invalid(): + responses.add(responses.GET, 'https://gist.githubusercontent.com/u/gone/raw', + body='Not Found', status=404) + with raises(ImportSourceInvalid) as exc: + fetch_source_text('https://gist.github.com/u/gone') + assert '404' in exc.value.detail + with raises(ImportSourceInvalid): # unregistered: ConnectionError + fetch_source_text('https://gist.github.com/u/unreachable') + with raises(ImportSourceInvalid): + fetch_source_text('https://docs.google.com/spreadsheets/nodoc') + with raises(ImportSourceInvalid): + fetch_source_text(' ') + + +# --------------------------------------------------------------------------- +# Parsing a source +# --------------------------------------------------------------------------- + +def _names(rows): + return [r['name'] for r in rows] + + +def test_parse_filename_csv(): + rows, columns = parse_source_rows(FIXTURE_FILENAME_CSV) + assert _names(rows) == [i['img_name'] for i in FIXTURE_FILE_INFOS] + assert [r['row'] for r in rows[:2]] == [2, 3] # header is row 1 + assert columns == {'name': 'filename', 'file_id': None, 'ignored': []} + + +def test_parse_full_csv_reads_only_the_name_column(): + """#509: metadata columns are ignored; Commons supplies them.""" + rows, columns = parse_source_rows(FIXTURE_FULL_CSV) + assert _names(rows) == [i['img_name'] for i in FIXTURE_FILE_INFOS] + assert columns['name'] == 'img_name' + assert 'img_width' in columns['ignored'] and 'img_name' not in columns['ignored'] + assert set(rows[0]) == {'row', 'name_as_written', 'file_id_as_written', + 'name', 'file_id', 'malformed_file_id'} + + +def test_parse_headers_case_bom_and_preference(): + rows, columns = parse_source_rows(u'\ufeffFile_ID, FileName ,img_name,File ID\n' + u'42,A b.jpg,Other.jpg,x\n') + assert columns == {'name': 'filename', 'file_id': 'file_id', + 'ignored': ['img_name', 'File ID']} + assert rows[0]['name'] == 'A_b.jpg' and rows[0]['file_id'] == 42 + assert rows[0]['name_as_written'] == 'A b.jpg' + + +def test_parse_rejects_columns_without_a_name_column(): + with raises(ImportSourceInvalid) as exc: + parse_source_rows('a,b\n1,2\n') + assert 'no filename or img_name column' in exc.value.detail + + +def test_parse_header_less_lists(): + # a gist: commas in names survive, File: prefixes go, blank lines keep numbers + rows, columns = parse_source_rows('Foo, bar.jpg\n\nFile:Baz qux.png\n') + assert [(r['row'], r['name']) for r in rows] == [(1, 'Foo,_bar.jpg'), + (3, 'Baz_qux.png')] + assert columns['name'] is None + # a one-column Google Sheet export: quoted cells + rows, _ = parse_source_rows('"Foo, bar.jpg"\n"Two.jpg"\n') + assert _names(rows) == ['Foo,_bar.jpg', 'Two.jpg'] + # one column with an unknown header word: read as a name (as before) + rows, _ = parse_source_rows('files\nOne.jpg\n') + assert _names(rows) == ['files', 'One.jpg'] + + +def test_parse_file_id_cells(): + assert parse_file_id('149673020') == (149673020, False) + assert parse_file_id(u' 12\xa0') == (12, False) + assert parse_file_id('') == (None, False) + assert parse_file_id(' ') == (None, False) + for bad in ['1.49673E+08', '149,673,020', '12.0', 'abc', '0', '-5', '1 2']: + assert parse_file_id(bad) == (None, True), bad + + +def test_parse_short_rows_and_malformed_ids(): + rows, _ = parse_source_rows('filename,file_id\nA.jpg\nB.jpg,1.2E+3\n,\n,7\n') + assert [(r['row'], r['name'], r['file_id'], r['malformed_file_id']) + for r in rows] == [(2, 'A.jpg', None, False), + (3, 'B.jpg', None, True), + (5, '', 7, False)] + + +def test_parse_name_list(): + rows = parse_name_list(['File:A b.jpg', '', ' ', 'C.jpg']) + assert [(r['row'], r['name']) for r in rows] == [(1, 'A_b.jpg'), (4, 'C.jpg')] + assert _names(parse_name_list('A.jpg\nB.jpg')) == ['A.jpg', 'B.jpg'] + with raises(ImportSourceInvalid): + parse_name_list(None) + + +def test_formula_guard_round_trip(): + for name in ['=SUM(A1).jpg', '+1.jpg', '-x.jpg', '@me.jpg', "'s-Hertogenbosch.jpg", + "''x.jpg", 'Plain.jpg']: + assert unguard_cell(guard_cell(name)) == name + assert normalise_name(guard_cell(name)) == name + assert guard_cell('=x.jpg') == "'=x.jpg" + assert normalise_name("'s-Hertogenbosch.jpg") == "'s-Hertogenbosch.jpg" def test_make_entry_reupload(): @@ -263,48 +334,25 @@ def fake(query, params, db_host=labs.COMMONS_DB_HOST): return fake -def test_load_by_filename_batches_lookups(monkeypatch): +def test_lookup_by_names_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} + names = ['File_%02d.jpg' % i for i in range(10)] calls = [] - _patch_replicas(monkeypatch, _fake_main_replica(existing, calls)) + _patch_replicas(monkeypatch, _fake_main_replica(set(names), calls)) - entries, warnings = loaders.load_by_filename(names, source='local') + found = loaders.lookup_by_names(names + names[:3], 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 == [] + assert sorted(found) == names -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_lookup_by_names_remote_keys_by_normalised_name(mock_external_apis): + from montage import loaders + from .conftest import SELECTED_FILE_INFO + name = normalise_name(SELECTED_FILE_INFO['img_name']) # remote: spaces + found = loaders.lookup_by_names([name, 'Not_there.jpg'], source='remote') + assert list(found) == [name] def test_round_source_params_hold_large_file_lists(): @@ -343,19 +391,6 @@ def test_get_files_order_is_binary_like_the_old_query(monkeypatch): 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). @@ -437,3 +472,95 @@ 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') + + +# --------------------------------------------------------------------------- +# Lookup by file_id (hatnote/montage#510): same query as by name, other column +# --------------------------------------------------------------------------- + +# _files_by_name's query as on master 939d890, copied verbatim: the shared +# query builder must not change the SQL that the name lookups send. +_MASTER_FILES_BY_NAME_SQL = ''' + SELECT DISTINCT {cols} + FROM commonswiki_p.file AS file + JOIN commonswiki_p.filerevision AS fr ON fr.fr_id = file.file_latest + AND fr.fr_deleted = 0 + 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} + WHERE file.file_name IN ({names}) + AND file.file_deleted = 0 + ''' + + +def test_files_by_name_sql_unchanged(): + from montage import labs + seen = [] + labs._files_by_name(lambda q, p: seen.append((q, p)) or [], ['A.jpg', 'B.jpg']) + expected = _MASTER_FILES_BY_NAME_SQL.format( + cols=', '.join(labs.FILE_COLS), + earliest_rev=labs._EARLIEST_REVISION_SUBQUERY, + names='%s, %s') + assert seen == [(expected, ('A.jpg', 'B.jpg'))] + + +def _fake_main_replica_by_id(existing, calls): + """existing: {file_id: name}; answers lookups by file_id.""" + from montage import labs + + def fake(query, params, db_host=labs.COMMONS_DB_HOST): + assert db_host == labs.COMMONS_DB_HOST + assert 'WHERE file.file_id IN (' in query + assert 'file.file_deleted = 0' in query and 'fr.fr_deleted = 0' in query + calls.append(params) + return [_file_row(existing[i], i) for i in params if i in existing] + return fake + + +def test_get_files_info_by_ids_batches_one_connection(monkeypatch): + from montage import labs + monkeypatch.setattr(labs, 'FILE_LOOKUP_CHUNK_SIZE', 3) + existing = {i: 'F%02d.jpg' % i for i in range(1, 9)} + calls = [] + opened = _patch_replicas(monkeypatch, _fake_main_replica_by_id(existing, calls)) + + found = labs.get_files_info_by_ids(['8', 7, 6, 5, 4, 3, 2, 1, 1, 99]) + + assert [len(c) for c in calls] == [3, 3, 3] # 9 distinct ids, as ints + assert all(isinstance(i, int) for c in calls for i in c) + assert sorted(found) == list(range(1, 9)) + assert found[3]['img_name'] == 'F03.jpg' + assert [(c.db_host, c.queries, c.closed) for c in opened] == [ + (labs.COMMONS_DB_HOST, 3, True)] + + +def test_get_files_info_by_ids_empty(monkeypatch): + from montage import labs + opened = _patch_replicas(monkeypatch, _fake_main_replica_by_id({}, [])) + assert labs.get_files_info_by_ids([]) == {} + assert opened == [] + + +@pytest.mark.xfail( + os.environ.get('TOOLFORGE') != '1', + reason='Requires live wikireplicas (Toolforge); set TOOLFORGE=1 to run', +) +def test_get_files_info_by_ids_live(): + """A file found by name is found again by its file_id, with the same + details.""" + from montage.labs import get_files_info_by_names, get_files_info_by_ids + name = 'Reynisfjara,_Suðurland,_Islandia,_2014-08-17,_DD_164.JPG' + by_name = get_files_info_by_names([name])[name] + by_id = get_files_info_by_ids([by_name['file_id']]) + assert by_id[by_name['file_id']] == by_name + + +def test_source_url_rewrites_only_gist_links(): + """#208: a plain CSV / text link got '/raw' appended and returned 404.""" + from montage.loaders import source_url + assert source_url('https://gist.github.com/u/abc') == ( + 'https://gist.githubusercontent.com/u/abc/raw', False) + assert source_url('https://gist.githubusercontent.com/u/abc/raw/x.csv') == ( + 'https://gist.githubusercontent.com/u/abc/raw/x.csv', False) + assert source_url('https://wikimedia.be/public/wlh/list.txt') == ( + 'https://wikimedia.be/public/wlh/list.txt', False) diff --git a/montage/utils.py b/montage/utils.py index f6dc323b..2db9997f 100644 --- a/montage/utils.py +++ b/montage/utils.py @@ -73,6 +73,37 @@ class NotImplementedResponse(MontageError, BadRequest, NotImplementedError): "Raised when a feature hasn't yet been implemented" +# Import check errors (hatnote/montage#510). They carry an error_type so the +# frontend can tell them apart; subclasses of InvalidAction (400), not of +# ValueError/TypeError, which older import code caught as "try another format". +class _ImportError(InvalidAction): + error_type = None + + def __init__(self, *args, **kwargs): + kwargs.setdefault('error_type', self.error_type) + super(_ImportError, self).__init__(*args, **kwargs) + + +class ImportSourceInvalid(_ImportError): + "The import source cannot be read (bad link, no name column, ...)" + error_type = 'import_source_invalid' + + +class ImportCheckBlocked(_ImportError): + "The checked list has rows that block the import" + error_type = 'import_check_blocked' + + +class ImportCheckExpired(_ImportError): + "The check token is unknown, expired or for another campaign/source" + error_type = 'import_check_expired' + + +class ImportCheckRequired(_ImportError): + "An import of a first-round source was sent without a check token" + error_type = 'import_check_required' + + DEFAULT_SERIES = {'name': 'Unofficial', 'description': 'For unofficial campaigns, whether for testing or just for fun!', 'url': 'TODO add docs url'} # TODO: status is always active @@ -159,6 +190,7 @@ def get_env_name(): 'debug': True, 'superusers': ['Slaporte', 'MahmoudHashemi'], 'dev_local_cookie_value': '"W7XGXxmUjl4kbkE0TWaFo4Oth50=?userid=NjAyNDQ3NA==&username=IlNsYXBvcnRlIg=="', + 'import_check_path': None, '__file__': 'devtest-builtin', '__env__': 'devtest', } @@ -203,6 +235,7 @@ def _bool(key, default=False): 'api_log_path': os.environ.get('MONTAGE_API_LOG_PATH', 'montage_api.log'), 'replay_log_path': os.environ.get('MONTAGE_REPLAY_LOG_PATH'), 'feel_log_path': os.environ.get('MONTAGE_FEEL_LOG_PATH'), + 'import_check_path': os.environ.get('MONTAGE_IMPORT_CHECK_PATH'), '__env__': env_name, '__file__': 'environment', }) From 399c5280d19ec59a5bf177e20cb9591ab3af4939 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:25:35 +0000 Subject: [PATCH 02/20] wip(#510): import_check module (classify, same-name keys, check files) Refs hatnote/montage#510, #645 Co-Authored-By: Claude Opus 5.5 --- montage/import_check.py | 524 +++++++++++++++++++++++++++++ montage/tests/test_import_check.py | 416 +++++++++++++++++++++++ 2 files changed, 940 insertions(+) create mode 100644 montage/import_check.py create mode 100644 montage/tests/test_import_check.py diff --git a/montage/import_check.py b/montage/import_check.py new file mode 100644 index 00000000..31b1f774 --- /dev/null +++ b/montage/import_check.py @@ -0,0 +1,524 @@ +# -*- coding: utf-8 -*- +"""Checked imports (hatnote/montage#510). + +Every first-round import source (CSV / gist / Google Sheet link, file +list, category) is first *checked*: fetched, looked up on Commons (by +file_id where a row has one, else by name) and classified row by row. The +result is kept as a JSON file under a random token; the import then reads +exactly that list, without fetching the source or asking Commons again. + +Row statuses: + + ok found on Commons, imported + renamed found by file_id under another name: imported under + Commons' current name (information) + duplicate the same Commons file as an earlier row: imported once + unknown_name no file_id and the name is not on Commons: left out, + warning + unknown_file_id the file_id is not on Commons: blocks + malformed_file_id the file_id is not a plain whole number: blocks + same_name a different Commons file whose name the database treats + as equal to another's (utf8mb4_unicode_ci): blocks a + list import (CSV, gist, Sheet, file list); in a category + the first file is kept and the others left out + +The check file holds the campaign id, the source, each row as written and +Commons' data per file (including Commons uploader names); no Montage user. +""" +from __future__ import absolute_import + +import os +import re +import csv +import json +import secrets +import datetime +import tempfile +import unicodedata +from io import StringIO +from collections import Counter + +from sqlalchemy import text + +from .loaders import (fetch_source_text, parse_source_rows, parse_name_list, + lookup_by_names, lookup_by_ids, category_records, + guard_cell) +from .utils import (PROJ_PATH, get_env_name, ImportSourceInvalid, + ImportCheckBlocked, ImportCheckExpired) + +FORMAT = 1 + +CHECKED_METHODS = ('csv', 'gistcsv', 'category', 'selected') +LIST_METHODS = ('csv', 'gistcsv', 'selected') # same-name pairs block these + +BLOCKING_STATUSES = ('unknown_file_id', 'malformed_file_id', 'same_name') +IMPORTED_STATUSES = ('ok', 'renamed') +ALL_STATUSES = ('ok', 'renamed', 'duplicate', 'unknown_name', + 'unknown_file_id', 'malformed_file_id', 'same_name') + +CHECK_MAX_AGE = datetime.timedelta(days=7) +ISSUES_SHOWN = 1000 # non-ok rows in the check response; the file has all +BLOCKING_ROWS_SHOWN = 10 # rows named in a "blocked" error +WARNING_ROWS_SHOWN = 50 # rows named per import warning + +# The same-name check (one switch). Two different files whose names the +# database compares as equal cannot both be stored where entries.name has a +# unique index (#645), and are easily confused by everyone else. +SAME_NAME_CHECK = True +SAME_NAME_COLLATION = 'utf8mb4_unicode_ci' # entries.name's collation +SAME_NAME_CHUNK_SIZE = 500 + +ENV_NAME = get_env_name() +DEFAULT_CHECK_DIR = os.path.join(PROJ_PATH, 'tmp', 'import_checks') +# deployed instances: the folder must be shared by all pods (and the import +# worker), so it has to be configured +CHECK_DIR_REQUIRED_ENVS = ('beta', 'prod', 'devlabs') + +_TOKEN_RE = re.compile(r'^[A-Za-z0-9_-]{32}$') + + +def lookup_source(): + """Where Commons is looked up: the wikireplica, except in local dev, + which asks production's public utils endpoint (as rdb.py does).""" + return 'remote' if ENV_NAME == 'dev' else 'local' + + +def _utcnow(): + return datetime.datetime.now(datetime.timezone.utc).replace(microsecond=0) + + +def _iso(dt): + return dt.strftime('%Y-%m-%dT%H:%M:%SZ') + + +def _parse_iso(value): + dt = datetime.datetime.strptime(value, '%Y-%m-%dT%H:%M:%SZ') + return dt.replace(tzinfo=datetime.timezone.utc) + + +# --------------------------------------------------------------------------- +# The check +# --------------------------------------------------------------------------- + +def run_check(request_dict, campaign_id, source='local', rdb_session=None, + now=None): + """Fetch, look up and classify an import source; returns the check + result (a JSON-serialisable dict). Writes nothing.""" + request_dict = request_dict or {} + import_method = request_dict.get('import_method') + if import_method not in CHECKED_METHODS: + raise ImportSourceInvalid('cannot check import method %r' % (import_method,)) + columns = {'name': None, 'file_id': None, 'ignored': []} + if import_method in ('csv', 'gistcsv'): + if import_method == 'gistcsv': + url = request_dict.get('gist_url') or request_dict.get('csv_url') + else: + url = request_dict.get('csv_url') + rows, columns = parse_source_rows(fetch_source_text(url)) + source_info = {'csv_url': url} + rows = _classify_list(rows, source) + elif import_method == 'selected': + rows = parse_name_list(request_dict.get('file_names')) + source_info = {'file_names': [r['name_as_written'] for r in rows]} + rows = _classify_list(rows, source) + else: + category = request_dict.get('category') + records = category_records(category, source) + source_info = {'category': category} + rows = [_row({'row': i + 1, 'name_as_written': rec['img_name'], + 'file_id_as_written': ''}, 'ok', rec=rec) + for i, rec in enumerate(records)] + _mark_duplicates(rows) + if SAME_NAME_CHECK: + _mark_same_names(rows, import_method, rdb_session) + now = now or _utcnow() + return {'format': FORMAT, + 'checked_at': _iso(now), + 'campaign_id': campaign_id, + 'import_method': import_method, + 'source': source_info, + 'lookup': source, + 'columns': columns, + 'rows': rows} + + +def _row(src, status, rec=None, reason='', file_id=None): + commons_name = rec['img_name'] if rec else None + if rec and rec.get('file_id') is not None: + file_id = int(rec['file_id']) + return {'row': src['row'], + 'name_as_written': src['name_as_written'], + 'file_id_as_written': src['file_id_as_written'], + 'commons_name': commons_name, + 'file_id': file_id, + 'status': status, + 'reason': reason, + 'group': None, + 'commons': rec} + + +def _classify_list(rows, source): + """Statuses of a CSV / file list's rows, looked up on Commons.""" + ids = [r['file_id'] for r in rows if r['file_id'] is not None] + by_id = lookup_by_ids(ids) if ids and source != 'remote' else {} + # dev (remote): the public utils endpoint has no lookup by file_id, so + # rows with a file_id are looked up by name and their ids compared + names = [r['name'] for r in rows + if r['name'] and not r['malformed_file_id'] + and (r['file_id'] is None or source == 'remote')] + by_name = lookup_by_names(names, source) + + ret = [] + for r in rows: + if r['malformed_file_id']: + ret.append(_row(r, 'malformed_file_id', + reason='file_id "%s" is not a plain whole number' + % r['file_id_as_written'].strip())) + elif r['file_id'] is not None and source == 'remote': + rec = by_name.get(r['name']) + if rec and rec.get('file_id') is not None and int(rec['file_id']) == r['file_id']: + ret.append(_row(r, 'ok', rec=rec)) + else: + ret.append(_row(r, 'unknown_file_id', file_id=r['file_id'], + reason='dev: compared by name only; file_id %s' + ' not confirmed' % r['file_id'])) + elif r['file_id'] is not None: + rec = by_id.get(r['file_id']) + if rec is None: + ret.append(_row(r, 'unknown_file_id', file_id=r['file_id'], + reason='file_id %s is not on Commons (deleted,' + ' or a wrong number)' % r['file_id'])) + elif not r['name']: + ret.append(_row(r, 'ok', rec=rec, + reason='no name given; Commons name used')) + elif rec['img_name'] == r['name']: + ret.append(_row(r, 'ok', rec=rec)) + else: + ret.append(_row(r, 'renamed', rec=rec, + reason=u'renamed on Commons: %s → %s' + % (r['name'], rec['img_name']))) + else: + rec = by_name.get(r['name']) + if rec is not None: + ret.append(_row(r, 'ok', rec=rec)) + else: + ret.append(_row(r, 'unknown_name', + reason='"%s" is not on Commons: check the' + ' spelling (renamed or deleted?)' % r['name'])) + return ret + + +def _file_key(row): + if row['file_id'] is not None: + return ('file_id', row['file_id']) + return ('name', row['commons_name']) + + +def _mark_duplicates(rows): + """The same Commons file listed twice (same name, or an old name and + its file_id): imported once, from its first row.""" + first_row = {} + for r in rows: + if r['status'] not in IMPORTED_STATUSES: + continue + key = _file_key(r) + if key in first_row: + r['status'] = 'duplicate' + r['reason'] = 'the same file as row %s' % first_row[key] + r['commons'] = None + else: + first_row[key] = r['row'] + + +def _mark_same_names(rows, import_method, rdb_session): + candidates = [r for r in rows if r['status'] in IMPORTED_STATUSES] + keys = same_name_keys([r['commons_name'] for r in candidates], rdb_session) + groups = {} + for r in candidates: + groups.setdefault(keys[r['commons_name']], []).append(r) + group_id = 0 + for members in groups.values(): + if len(members) < 2: + continue + group_id += 1 + if import_method in LIST_METHODS: + for r in members: + others = [m for m in members if m is not r] + r['status'] = 'same_name' + r['group'] = group_id + r['reason'] = ('the database treats this name as equal to: %s' + % ', '.join('row %s %s' % (m['row'], m['commons_name']) + for m in others)) + r['commons'] = None + else: + # category: keep the first in binary name order, as before + members.sort(key=lambda m: m['commons_name'].encode('utf8')) + kept = members[0] + kept['group'] = group_id + for r in members[1:]: + r['status'] = 'same_name' + r['group'] = group_id + r['reason'] = ('left out: the database treats this name as' + ' equal to %s (kept)' % kept['commons_name']) + r['commons'] = None + + +def same_name_keys(names, rdb_session=None): + """{name: comparison key}: two names with equal keys are the same name + for Montage's database (entries.name, utf8mb4_unicode_ci). + + On MariaDB/MySQL the database computes the keys itself with + WEIGHT_STRING, so they are exactly its comparison (no table is read). + Anywhere else (SQLite in tests and local dev) a Python approximation is + used, for tests only: it folds case and drops accents, so е/ё and й/и + compare equal as in utf8mb4_unicode_ci, but ß/ss, æ/ae and other + expansions do not.""" + names = sorted(set(names)) + if not names: + return {} + if rdb_session is not None and _dialect_name(rdb_session) == 'mysql': + return _database_keys(rdb_session, names) + return {name: _test_only_approximate_key(name) for name in names} + + +def _dialect_name(rdb_session): + try: + return rdb_session.get_bind().dialect.name + except Exception: + return None + + +def _database_keys(rdb_session, names): + ret = {} + for i in range(0, len(names), SAME_NAME_CHUNK_SIZE): + chunk = names[i:i + SAME_NAME_CHUNK_SIZE] + cols = ', '.join('WEIGHT_STRING(CONVERT(:n%d USING utf8mb4) COLLATE %s) AS w%d' + % (j, SAME_NAME_COLLATION, j) for j in range(len(chunk))) + params = {'n%d' % j: name for j, name in enumerate(chunk)} + row = rdb_session.execute(text('SELECT ' + cols), params).fetchone() + for j, name in enumerate(chunk): + ret[name] = bytes(row[j]) + return ret + + +def _test_only_approximate_key(name): + """TEST-ONLY stand-in for the database's utf8mb4_unicode_ci comparison + (SQLite has none). Never used against MariaDB.""" + decomposed = unicodedata.normalize('NFKD', name) + stripped = u''.join(c for c in decomposed if not unicodedata.combining(c)) + return stripped.lower() + + +# --------------------------------------------------------------------------- +# Reading a check result +# --------------------------------------------------------------------------- + +def counts(result): + found = Counter(r['status'] for r in result['rows']) + return {status: found.get(status, 0) for status in ALL_STATUSES} + + +def blocking_rows(result): + """Rows that block the import, recomputed from the rows.""" + if result['import_method'] not in LIST_METHODS: + return [] + return [r for r in result['rows'] if r['status'] in BLOCKING_STATUSES] + + +def importable(result): + return [r for r in result['rows'] + if r['status'] in IMPORTED_STATUSES and r['commons'] is not None] + + +def raise_if_blocked(result): + blocked = blocking_rows(result) + if not blocked: + return + per_status = Counter(r['status'] for r in blocked) + detail = '%s rows block this import (%s). Fix them in the source and check again.' % ( + len(blocked), ', '.join('%s %s' % (n, s.replace('_', ' ')) + for s, n in sorted(per_status.items()))) + lines = ['row %s: %s' % (r['row'], r['reason']) for r in blocked[:BLOCKING_ROWS_SHOWN]] + if len(blocked) > BLOCKING_ROWS_SHOWN: + lines.append('... and %s more' % (len(blocked) - BLOCKING_ROWS_SHOWN)) + raise ImportCheckBlocked(detail + '\n' + '\n'.join(lines)) + + +def _listed(lines): + shown = [u'- ' + line for line in lines[:WARNING_ROWS_SHOWN]] + if len(lines) > WARNING_ROWS_SHOWN: + shown.append(u'- ... and %s more' % (len(lines) - WARNING_ROWS_SHOWN)) + return u'\n'.join(shown) + + +def import_warnings(result): + """Warnings for the import response: a list of one-key dicts, like + the other import warnings.""" + ret = [] + rows = result['rows'] + unknown = [r['reason'] for r in rows if r['status'] == 'unknown_name'] + if unknown: + ret.append({'import issues': u'unable to load %s files:\n%s' + % (len(unknown), _listed(unknown))}) + renamed = [r['reason'] for r in rows if r['status'] == 'renamed'] + if renamed: + ret.append({'renamed': u'%s files are imported under their current' + u' name on Commons:\n%s' % (len(renamed), _listed(renamed))}) + left_out = [u'%s (%s)' % (r['commons_name'], r['reason']) for r in rows + if r['status'] == 'same_name'] + if left_out: + ret.append({'same name': u'%s files left out:\n%s' + % (len(left_out), _listed(left_out))}) + return ret + + +def source_params(result): + """round_sources.params for the import, from the check (not from the + import request).""" + return dict(result['source']) + + +def describe_source(result): + src = result['source'] + if 'category' in src: + return 'category (%s)' % src['category'] + if 'csv_url' in src: + return 'csv (%r)' % src['csv_url'] + return 'file list (%s names)' % len(src.get('file_names', [])) + + +def summarize(result, token): + """The check endpoint's response.""" + issues = [{k: v for k, v in r.items() if k != 'commons'} + for r in result['rows'] if r['status'] != 'ok'] + groups = {} + for r in result['rows']: + if r['group'] is not None: + groups.setdefault(r['group'], []).append( + {'row': r['row'], 'name_as_written': r['name_as_written'], + 'commons_name': r['commons_name'], 'file_id': r['file_id'], + 'status': r['status']}) + checked_at = _parse_iso(result['checked_at']) + return {'token': token, + 'checked_at': result['checked_at'], + 'expires_at': _iso(checked_at + CHECK_MAX_AGE), + 'import_method': result['import_method'], + 'source': result['source'], + 'columns': result['columns'], + 'counts': counts(result), + 'blocking': bool(blocking_rows(result)), + 'total_rows': len(result['rows']), + 'importable_count': len(importable(result)), + 'issues': issues[:ISSUES_SHOWN], + 'issues_total': len(issues), + 'issues_truncated': len(issues) > ISSUES_SHOWN, + 'same_name_groups': [groups[k] for k in sorted(groups)]} + + +def upload_csv(result): + """The download: a ready-to-use upload file with the columns filename + (Commons' current name) and file_id, one row per file the import would + take. Uploading it again passes the check unchanged. Cells that a + spreadsheet would run as a formula get a leading ' (removed again when + Montage reads the file).""" + out = StringIO() + writer = csv.writer(out, lineterminator='\n') + writer.writerow(['filename', 'file_id']) + for r in importable(result): + writer.writerow([guard_cell(r['commons_name']), + '' if r['file_id'] is None else r['file_id']]) + return out.getvalue().encode('utf8') + + +# --------------------------------------------------------------------------- +# Check files +# --------------------------------------------------------------------------- + +def check_dir(config): + return ((config or {}).get('import_check_path') + or os.environ.get('MONTAGE_IMPORT_CHECK_PATH') + or DEFAULT_CHECK_DIR) + + +def check_dir_problem(config, env_name): + """Why the app must not start, or None.""" + configured = ((config or {}).get('import_check_path') + or os.environ.get('MONTAGE_IMPORT_CHECK_PATH')) + if env_name in CHECK_DIR_REQUIRED_ENVS and not configured: + return ('MONTAGE_IMPORT_CHECK_PATH is not set: env %r needs a folder for' + ' import checks that all pods share, e.g.' + ' /data/project//import_checks' % env_name) + return None + + +def _cleanup(folder, now): + cutoff = (now - CHECK_MAX_AGE).timestamp() + for name in os.listdir(folder): + if not (name.endswith('.json') or name.startswith('.tmp-')): + continue + path = os.path.join(folder, name) + try: + if os.path.getmtime(path) < cutoff: + os.remove(path) + except FileNotFoundError: # another process cleaned up first + pass + + +def save(result, config, now=None): + """Write the check result to a new check file; returns its token. + Deletes check files older than CHECK_MAX_AGE first.""" + folder = check_dir(config) + os.makedirs(folder, exist_ok=True) + os.chmod(folder, 0o700) + _cleanup(folder, now or _utcnow()) + token = secrets.token_urlsafe(24) + fd, tmp_path = tempfile.mkstemp(dir=folder, prefix='.tmp-', suffix='.json') + try: + os.fchmod(fd, 0o600) + with os.fdopen(fd, 'w', encoding='utf-8') as f: + json.dump(result, f, separators=(',', ':'), ensure_ascii=False, + default=str) + f.flush() + os.fsync(f.fileno()) + os.replace(tmp_path, os.path.join(folder, token + '.json')) + except BaseException: + try: + os.remove(tmp_path) + except OSError: + pass + raise + return token + + +def _method_family(import_method): + return 'csv' if import_method in ('csv', 'gistcsv') else import_method + + +def load(token, config, campaign_id, import_method=None, now=None): + """The check result for a token, if it exists, is younger than + CHECK_MAX_AGE and was made for this campaign (and import method).""" + expired = 'check not found or expired, please check the source again' + if not isinstance(token, str) or not _TOKEN_RE.match(token): + raise ImportCheckExpired(expired) + folder = os.path.realpath(check_dir(config)) + path = os.path.realpath(os.path.join(folder, token + '.json')) + if not path.startswith(folder + os.sep): + raise ImportCheckExpired(expired) + try: + with open(path, encoding='utf-8') as f: + result = json.load(f) + checked_at = _parse_iso(result['checked_at']) + except (OSError, ValueError, KeyError, TypeError): + raise ImportCheckExpired(expired) + if result.get('format') != FORMAT: + raise ImportCheckExpired(expired) + if (now or _utcnow()) - checked_at > CHECK_MAX_AGE: + raise ImportCheckExpired('check expired (older than 7 days), please' + ' check the source again') + if result.get('campaign_id') != campaign_id: + raise ImportCheckExpired('this check was made for another campaign,' + ' please check the source again') + if import_method and _method_family(result.get('import_method')) != _method_family(import_method): + raise ImportCheckExpired('this check was made for another import' + ' source, please check the source again') + return result diff --git a/montage/tests/test_import_check.py b/montage/tests/test_import_check.py new file mode 100644 index 00000000..2b52a7d1 --- /dev/null +++ b/montage/tests/test_import_check.py @@ -0,0 +1,416 @@ +# -*- coding: utf-8 -*- +"""The import check (hatnote/montage#510): classification, the same-name +rule, check files. Commons lookups are stand-ins; all data is synthetic, +except the е/ё pair from #645 (public Commons file names and ids).""" +import csv +import io +import json +import os +import time +import datetime + +import pytest +import responses as responses_lib + +from montage import import_check +from montage.import_check import (run_check, counts, blocking_rows, importable, + raise_if_blocked, import_warnings, summarize, + upload_csv, save, load, same_name_keys) +from montage.loaders import parse_source_rows +from montage.utils import (ImportCheckBlocked, ImportCheckExpired, + ImportSourceInvalid) + +CAMPAIGN = 7 +VOLOCHEK = u'Церковь_Богоявления,_Вышний_Волочек.jpg' +VOLOCHYOK = u'Церковь_Богоявления,_Вышний_Волочёк.jpg' + + +def info(name, file_id, **kw): + rec = {'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} + rec.update(kw) + return rec + + +class FakeCommons(object): + """Stands in for the wikireplica lookups (local mode).""" + def __init__(self, files): + self.by_name = {f['img_name']: f for f in files} + self.by_id = {f['file_id']: f for f in files} + self.calls = [] + + def names(self, names, source='local'): + self.calls.append(('names', sorted(set(names)))) + return {n: self.by_name[n] for n in names if n in self.by_name} + + def ids(self, ids): + self.calls.append(('ids', sorted(set(ids)))) + return {i: self.by_id[i] for i in ids if i in self.by_id} + + def category(self, name, source='local'): + self.calls.append(('category', name)) + return sorted(self.by_name.values(), key=lambda f: f['img_name'].encode('utf8')) + + +@pytest.fixture +def commons(monkeypatch): + fake = FakeCommons([]) + + def install(files): + fake.__init__(files) + return fake + monkeypatch.setattr(import_check, 'lookup_by_names', fake.names) + monkeypatch.setattr(import_check, 'lookup_by_ids', fake.ids) + monkeypatch.setattr(import_check, 'category_records', fake.category) + return install + + +def csv_check(monkeypatch, text, method='csv'): + monkeypatch.setattr(import_check, 'fetch_source_text', lambda url: text) + return run_check({'import_method': method, 'csv_url': 'https://example.org/x.csv'}, + CAMPAIGN, source='local') + + +def statuses(result): + return [(r['row'], r['status']) for r in result['rows']] + + +# --------------------------------------------------------------------------- +# Classification +# --------------------------------------------------------------------------- + +def test_classify_every_status(monkeypatch, commons): + commons([info('A.jpg', 1), info('B_new.jpg', 2), info('C.jpg', 3)]) + result = csv_check(monkeypatch, 'filename,file_id\n' + 'A.jpg,1\n' # 2 ok + 'B old.jpg,2\n' # 3 renamed + 'C.jpg,\n' # 4 ok by name + 'Gone.jpg,\n' # 5 unknown name + 'X.jpg,99\n' # 6 unknown file_id + 'Y.jpg,1.2E+3\n' # 7 malformed + 'File:A.jpg,\n' # 8 duplicate (same name) + ',3\n') # 9 duplicate (id only) + assert statuses(result) == [(2, 'ok'), (3, 'renamed'), (4, 'ok'), + (5, 'unknown_name'), (6, 'unknown_file_id'), + (7, 'malformed_file_id'), (8, 'duplicate'), + (9, 'duplicate')] + renamed = result['rows'][1] + assert renamed['commons_name'] == 'B_new.jpg' and renamed['file_id'] == 2 + assert u'B_old.jpg → B_new.jpg' in renamed['reason'] + assert result['rows'][7]['reason'] == 'the same file as row 4' + assert [r['commons_name'] for r in importable(result)] == ['A.jpg', 'B_new.jpg', 'C.jpg'] + assert [r['row'] for r in blocking_rows(result)] == [6, 7] + assert counts(result)['duplicate'] == 2 + + +def test_old_name_and_its_file_id_are_one_file(monkeypatch, commons): + commons([info('New.jpg', 5)]) + result = csv_check(monkeypatch, 'filename,file_id\nNew.jpg,\nOld.jpg,5\n') + assert statuses(result) == [(2, 'ok'), (3, 'duplicate')] + + +def test_no_name_with_a_known_file_id_is_ok(monkeypatch, commons): + commons([info('A.jpg', 1)]) + result = csv_check(monkeypatch, 'filename,file_id\n,1\n') + assert statuses(result) == [(2, 'ok')] + assert result['rows'][0]['reason'] == 'no name given; Commons name used' + + +def test_full_csv_metadata_is_ignored(monkeypatch, commons): + """#509: Commons' metadata wins over the CSV's.""" + commons([info('A.jpg', 1, img_width=4000)]) + result = csv_check(monkeypatch, 'img_name,img_width,img_user_text\nA.jpg,1,Someone\n') + assert importable(result)[0]['commons']['img_width'] == 4000 + assert result['columns']['ignored'] == ['img_width', 'img_user_text'] + + +def test_blocking_per_method(monkeypatch, commons): + commons([info('A.jpg', 1)]) + result = csv_check(monkeypatch, 'filename,file_id\nA.jpg,77\n') + assert result['rows'][0]['status'] == 'unknown_file_id' + with pytest.raises(ImportCheckBlocked) as exc: + raise_if_blocked(result) + assert exc.value.detail.startswith('1 rows block this import (1 unknown file id)') + assert 'row 2: file_id 77 is not on Commons' in exc.value.detail + + +def test_unknown_names_do_not_block(monkeypatch, commons): + commons([info('A.jpg', 1)]) + result = csv_check(monkeypatch, 'A.jpg\nGone.jpg\n') + raise_if_blocked(result) + warnings = import_warnings(result) + assert list(warnings[0]) == ['import issues'] + assert 'Gone.jpg' in warnings[0]['import issues'] + + +def test_blocked_detail_lists_ten_rows_count_first(monkeypatch, commons): + commons([]) + result = csv_check(monkeypatch, 'filename,file_id\n' + + ''.join('F%d.jpg,%d\n' % (i, 500 + i) for i in range(30))) + with pytest.raises(ImportCheckBlocked) as exc: + raise_if_blocked(result) + assert exc.value.detail.startswith('30 rows block this import') + assert '... and 20 more' in exc.value.detail + + +def test_lookups_are_batched_not_per_row(monkeypatch, commons): + fake = commons([info('A.jpg', 1), info('B.jpg', 2)]) + csv_check(monkeypatch, 'filename,file_id\nA.jpg,1\nB.jpg,\nC.jpg,\n') + assert fake.calls == [('ids', [1]), ('names', ['B.jpg', 'C.jpg'])] + + +def test_file_list_rows_are_lines(commons): + commons([info('A_b.jpg', 1)]) + result = run_check({'import_method': 'selected', + 'file_names': ['File:A b.jpg', '', 'Gone.jpg']}, + CAMPAIGN, source='local') + assert statuses(result) == [(1, 'ok'), (3, 'unknown_name')] + assert result['source'] == {'file_names': ['File:A b.jpg', 'Gone.jpg']} + + +def test_missing_source_is_import_source_invalid(): + for request in [{'import_method': 'selected'}, + {'import_method': 'nonsense'}, + {'import_method': 'csv'}, + {'import_method': 'category', 'category': ' '}]: + with pytest.raises(ImportSourceInvalid): + run_check(request, CAMPAIGN, source='local') + + +# --------------------------------------------------------------------------- +# dev (remote): no lookup by file_id; compared by name +# --------------------------------------------------------------------------- + +def test_remote_compares_file_ids_by_name(monkeypatch, commons): + fake = commons([info('A.jpg', 1), info('B.jpg', 2)]) + monkeypatch.setattr(import_check, 'fetch_source_text', + lambda url: 'filename,file_id\nA.jpg,1\nB.jpg,3\n') + result = run_check({'import_method': 'csv', 'csv_url': 'x'}, CAMPAIGN, source='remote') + assert statuses(result) == [(2, 'ok'), (3, 'unknown_file_id')] + assert result['rows'][1]['reason'].startswith('dev: compared by name only') + assert [c[0] for c in fake.calls] == ['names'] # never by id + + +# --------------------------------------------------------------------------- +# The gist fix: a gist is looked up where the instance looks up (no POST to +# production's utils endpoint from beta/prod) +# --------------------------------------------------------------------------- + +def test_gist_local_makes_no_remote_lookup(monkeypatch): + from montage import labs + monkeypatch.setattr(labs, 'get_files_info_by_names', + lambda names: {'A.jpg': info('A.jpg', 1)}) + monkeypatch.setattr('montage.loaders.get_files_info_by_names', + labs.get_files_info_by_names) + with responses_lib.RequestsMock() as rsps: + rsps.add(responses_lib.GET, 'https://gist.githubusercontent.com/u/abc/raw', + body='filename\nA.jpg\n', status=200) + result = run_check({'import_method': 'gistcsv', + 'gist_url': 'https://gist.github.com/u/abc'}, + CAMPAIGN, source='local') + assert len(rsps.calls) == 1 # the gist itself, no utils POST + assert statuses(result) == [(2, 'ok')] + + +# --------------------------------------------------------------------------- +# Same name (#645): names the database treats as equal +# --------------------------------------------------------------------------- + +def test_same_name_pair_blocks_a_list_import(monkeypatch, commons): + commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022), + info('Other.jpg', 3)]) + result = csv_check(monkeypatch, u'filename,file_id\n"%s",149673020\n"%s",\nOther.jpg,\n' + % (VOLOCHEK, VOLOCHYOK)) + assert statuses(result) == [(2, 'same_name'), (3, 'same_name'), (4, 'ok')] + assert result['rows'][0]['group'] == result['rows'][1]['group'] == 1 + assert VOLOCHYOK in result['rows'][0]['reason'] + assert [r['row'] for r in blocking_rows(result)] == [2, 3] + groups = summarize(result, 'x' * 32)['same_name_groups'] + assert [[m['commons_name'] for m in g] for g in groups] == [[VOLOCHEK, VOLOCHYOK]] + + +@pytest.mark.parametrize('pair', [('Photo.jpg', 'Photo.JPG'), + (u'Église.jpg', u'Eglise.jpg'), + (u'Йошкар.jpg', u'Иошкар.jpg')]) +def test_same_name_case_and_accents(monkeypatch, commons, pair): + commons([info(pair[0], 1), info(pair[1], 2)]) + result = csv_check(monkeypatch, 'filename\n%s\n%s\n' % pair) + assert [s for _, s in statuses(result)] == ['same_name', 'same_name'] + + +def test_same_name_in_a_category_keeps_the_first(commons): + commons([info(VOLOCHYOK, 149673022), info(VOLOCHEK, 149673020), info('A.jpg', 1)]) + result = run_check({'import_method': 'category', 'category': 'Cat'}, + CAMPAIGN, source='local') + by_name = {r['commons_name']: r['status'] for r in result['rows']} + # binary order: е (U+0435) sorts before ё (U+0451) + assert by_name == {VOLOCHEK: 'ok', VOLOCHYOK: 'same_name', 'A.jpg': 'ok'} + assert blocking_rows(result) == [] + warning = import_warnings(result)[-1]['same name'] + assert VOLOCHEK in warning and VOLOCHYOK in warning + + +def test_same_name_switch_off(monkeypatch, commons): + monkeypatch.setattr(import_check, 'SAME_NAME_CHECK', False) + commons([info(VOLOCHEK, 1), info(VOLOCHYOK, 2)]) + result = csv_check(monkeypatch, u'filename\n"%s"\n"%s"\n' % (VOLOCHEK, VOLOCHYOK)) + assert [s for _, s in statuses(result)] == ['ok', 'ok'] + + +def test_test_only_key_does_not_cover_expansions(): + """Documented gap of the SQLite stand-in (MariaDB's own keys are used + in production): ß/ss are not grouped.""" + keys = same_name_keys([u'Straße.jpg', u'Strasse.jpg']) + assert keys[u'Straße.jpg'] != keys[u'Strasse.jpg'] + + +def test_same_name_keys_ask_mariadb(monkeypatch): + """On MariaDB the keys come from WEIGHT_STRING, batched, no table.""" + sent = [] + + class Result(object): + def __init__(self, n): + self.n = n + + def fetchone(self): + return [b'k' for _ in range(self.n)] + + class Session(object): + def get_bind(self): + class Bind(object): + class dialect(object): + name = 'mysql' + return Bind() + + def execute(self, statement, params): + sent.append((str(statement), params)) + return Result(len(params)) + + monkeypatch.setattr(import_check, 'SAME_NAME_CHUNK_SIZE', 2) + keys = same_name_keys(['c.jpg', 'a.jpg', 'b.jpg'], Session()) + assert set(keys) == {'a.jpg', 'b.jpg', 'c.jpg'} + assert [len(p) for _, p in sent] == [2, 1] + sql = sent[0][0] + assert sql.startswith('SELECT WEIGHT_STRING(CONVERT(:n0 USING utf8mb4)' + ' COLLATE utf8mb4_unicode_ci) AS w0') + assert 'FROM' not in sql + + +# --------------------------------------------------------------------------- +# Summary and download +# --------------------------------------------------------------------------- + +def test_summary_caps_issues(monkeypatch, commons): + monkeypatch.setattr(import_check, 'ISSUES_SHOWN', 3) + commons([]) + result = csv_check(monkeypatch, ''.join('G%d.jpg\n' % i for i in range(5))) + summary = summarize(result, 't' * 32) + assert len(summary['issues']) == 3 and summary['issues_total'] == 5 + assert summary['issues_truncated'] and not summary['blocking'] + assert summary['importable_count'] == 0 and summary['total_rows'] == 5 + assert 'commons' not in summary['issues'][0] + assert summary['expires_at'] > summary['checked_at'] + + +def test_download_is_an_upload_file_that_passes_unchanged(monkeypatch, commons): + commons([info('A.jpg', 1), info('New.jpg', 2), info('=SUM(A1).jpg', 3)]) + first = csv_check(monkeypatch, 'img_name,file_id,img_width\n' + 'A.jpg,1,5\nOld.jpg,2,5\nA.jpg,,5\nGone.jpg,,5\n=SUM(A1).jpg,,5\n') + data = upload_csv(first).decode('utf8') + assert list(csv.reader(io.StringIO(data))) == [ + ['filename', 'file_id'], ['A.jpg', '1'], ['New.jpg', '2'], ["'=SUM(A1).jpg", '3']] + second = csv_check(monkeypatch, data) + assert [s for _, s in statuses(second)] == ['ok', 'ok', 'ok'] + assert upload_csv(second) == upload_csv(first) + + +# --------------------------------------------------------------------------- +# Check files +# --------------------------------------------------------------------------- + +@pytest.fixture +def config(tmpdir): + return {'import_check_path': str(tmpdir.join('checks'))} + + +def _result(campaign_id=CAMPAIGN, method='csv', checked_at=None): + now = checked_at or datetime.datetime.now(datetime.timezone.utc) + return {'format': 1, 'checked_at': now.strftime('%Y-%m-%dT%H:%M:%SZ'), + 'campaign_id': campaign_id, 'import_method': method, + 'source': {'csv_url': 'x'}, 'lookup': 'local', + 'columns': {}, 'rows': []} + + +def test_save_and_load(config): + token = save(_result(), config) + assert len(token) == 32 + folder = config['import_check_path'] + assert os.stat(folder).st_mode & 0o777 == 0o700 + path = os.path.join(folder, token + '.json') + assert os.stat(path).st_mode & 0o777 == 0o600 + assert os.listdir(folder) == [token + '.json'] # no temp file left + assert load(token, config, CAMPAIGN, 'gistcsv')['campaign_id'] == CAMPAIGN + + +def test_load_refuses_bad_tokens(config): + token = save(_result(), config) + old = save(_result(checked_at=datetime.datetime.now(datetime.timezone.utc) + - datetime.timedelta(days=8)), config) + for bad, args in [('../x', (CAMPAIGN,)), + ('../' + token[3:], (CAMPAIGN,)), + ('A' * 32, (CAMPAIGN,)), + (None, (CAMPAIGN,)), + (old, (CAMPAIGN,)), + (token, (CAMPAIGN + 1,)), + (token, (CAMPAIGN, 'category'))]: + with pytest.raises(ImportCheckExpired): + load(bad, config, *args) + + +def test_save_cleans_up_old_files(config, tmpdir): + keep = save(_result(), config) + stale = save(_result(), config) + folder = config['import_check_path'] + week_ago = time.time() - 8 * 24 * 3600 + os.utime(os.path.join(folder, stale + '.json'), (week_ago, week_ago)) + other = os.path.join(folder, 'notes.txt') + open(other, 'w').close() + os.utime(other, (week_ago, week_ago)) + newest = save(_result(), config) + assert sorted(os.listdir(folder)) == sorted([keep + '.json', newest + '.json', 'notes.txt']) + + +def test_check_file_has_no_montage_user(monkeypatch, commons, config): + commons([info('A.jpg', 1, img_user_text='CommonsUploader')]) + token = save(csv_check(monkeypatch, 'A.jpg\n'), config) + with open(os.path.join(config['import_check_path'], token + '.json')) as f: + raw = f.read() + stored = json.loads(raw) + assert set(stored) == {'format', 'checked_at', 'campaign_id', 'import_method', + 'source', 'lookup', 'columns', 'rows'} + assert 'CommonsUploader' in raw # Commons data is kept (approved) + assert 'Yarl' not in raw and 'Slaporte' not in raw + + +def test_check_dir_required_on_deployed_instances(monkeypatch): + monkeypatch.delenv('MONTAGE_IMPORT_CHECK_PATH', raising=False) + for env in ('beta', 'prod', 'devlabs'): + assert 'MONTAGE_IMPORT_CHECK_PATH' in import_check.check_dir_problem({}, env) + assert import_check.check_dir_problem({'import_check_path': '/x'}, env) is None + assert import_check.check_dir_problem({}, 'dev') is None + monkeypatch.setenv('MONTAGE_IMPORT_CHECK_PATH', '/y') + assert import_check.check_dir_problem({}, 'prod') is None + + +def test_header_less_list_with_comma_names(monkeypatch, commons): + """A gist that lists the #645 pair one per line, unquoted: the commas + stay in the names and the pair is found.""" + commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022)]) + result = csv_check(monkeypatch, u'%s\n%s\n' % (VOLOCHEK.replace('_', ' '), VOLOCHYOK)) + assert [(r['row'], r['commons_name'], r['status']) for r in result['rows']] == [ + (1, VOLOCHEK, 'same_name'), (2, VOLOCHYOK, 'same_name')] From 0befcf1bbdddea549c286daace1a5bd89de1fa95 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:28:46 +0000 Subject: [PATCH 03/20] wip(#510): check/download endpoints, import only by check token - POST /admin/campaign//import/check, GET .../import/check//download - import_entries requires check_token for csv/gistcsv/category/selected and imports exactly the checked list; blocked check -> 400 before writes - add_checked_entries replaces the three DAO loaders; file_id lookup in every campaign (FILE_ID_LOOKUP_CUTOFF removed) - app refuses to start on beta/prod/devlabs without MONTAGE_IMPORT_CHECK_PATH - no 'all disqualified' warning for an empty import (#208) - tests import through check + token Refs hatnote/montage#510, #208 Co-Authored-By: Claude Opus 5.5 --- montage/admin_endpoints.py | 113 ++++++++++++++++++--------- montage/app.py | 7 ++ montage/rdb.py | 77 ++++-------------- montage/tests/test_import_entries.py | 28 ++++--- montage/tests/test_web_basic.py | 67 ++++++++++------ tools/admin.py | 36 ++++++--- 6 files changed, 181 insertions(+), 147 deletions(-) diff --git a/montage/admin_endpoints.py b/montage/admin_endpoints.py index a8faabfc..2595c018 100644 --- a/montage/admin_endpoints.py +++ b/montage/admin_endpoints.py @@ -11,8 +11,11 @@ from .utils import (format_date, get_threshold_map, InvalidAction, + ImportCheckRequired, NotImplementedResponse, js_isoparse) +from . import import_check +from .loaders import make_entry from .rdb import (FINALIZED_STATUS, CoordinatorDAO, @@ -56,6 +59,9 @@ def get_admin_routes(): POST('/admin/campaign//publish', publish_report), POST('/admin/campaign//unpublish', unpublish_report), GET('/admin/campaign//audit', get_campaign_log), + POST('/admin/campaign//import/check', check_import), + GET('/admin/campaign//import/check//download', + download_import_check), POST('/admin/round//import', import_entries), POST('/admin/round//activate', activate_round), POST('/admin/round//pause', pause_round), @@ -304,21 +310,60 @@ def get_campaign_log(user_dao, campaign_id, request_dict): return {'data': ret} -def import_entries(user_dao, round_id, request_dict): +def check_import(user_dao, campaign_id, request_dict, config): """ - Summary: Load entries into a round via one of four import methods + Summary: Check a first-round import source before importing it (#510) + + Fetches the source, looks every row up on Commons (by file_id where a + row has one, else by name) and classifies the rows. Writes the result + as a check file and returns a summary with its token; the import then + takes exactly that list (import_entries with check_token). Writes + nothing to the database. + + Request model: as import_entries: import_method (csv, gistcsv, + category, selected) and csv_url / gist_url / category / file_names. + + Response model name: + - data: token, checked_at, expires_at, counts (per status), blocking, + total_rows, importable_count, columns, issues (rows that are not + ok, at most 1000), issues_total, issues_truncated, + same_name_groups + """ + coord_dao = CoordinatorDAO.from_campaign(user_dao, campaign_id) + result = import_check.run_check(request_dict, coord_dao.campaign.id, + source=import_check.lookup_source(), + rdb_session=user_dao.rdb_session) + token = import_check.save(result, config) + return {'data': import_check.summarize(result, token)} + + +def download_import_check(user_dao, campaign_id, token, config): + """The checked list as a ready-to-use upload file (filename, file_id).""" + coord_dao = CoordinatorDAO.from_campaign(user_dao, campaign_id) + result = import_check.load(token, config, coord_dao.campaign.id) + output_name = 'montage_import-%s-%s.csv' % (campaign_id, + result['checked_at'][:10]) + resp = Response(import_check.upload_csv(result), mimetype='text/csv') + resp.mimetype_params['charset'] = 'utf-8' + resp.headers['Content-Disposition'] = 'attachment; filename=%s' % (output_name,) + return resp + + +def import_entries(user_dao, round_id, request_dict, config): + """ + Summary: Load entries into a round via one of five import methods Request model: - round_id (in path) - import_method: - - gistcsv + - csv / gistcsv (a CSV, gist or Google Sheet link) - category + - selected (a list of file names) - round - - selected - - gist_url (if import_method=gistcsv) - - category (if import_method=category) - - threshold (if import_method=round) - - file_names (if import_method=selected) + - check_token: from /admin/campaign//import/check; required for + every method except round (#510). The import takes exactly the + checked list; the source is not fetched again. + - threshold, previous_round_id (if import_method=round) Response model name: - data: @@ -329,6 +374,9 @@ def import_entries(user_dao, round_id, request_dict): - status: success or failure - errors: description of the failure (if any) - warnings: possible problems to alert the user + - import issues (files not on Commons, left out) + - renamed (imported under their current Commons name) + - same name (category files left out, #645) - empty import (no entries) - duplicate import (no new entries) - all disqualified @@ -339,40 +387,28 @@ def import_entries(user_dao, round_id, request_dict): # loader warnings import_warnings = list() - if import_method == 'csv' or import_method == 'gistcsv': - if import_method == 'gistcsv': - csv_url = request_dict['gist_url'] - else: - csv_url = request_dict['csv_url'] - - entries, warnings = coord_dao.add_entries_from_csv(round_id, - csv_url) - params = {'csv_url': csv_url} - if warnings: - msg = u'unable to load {} files ({!r})'.format(len(warnings), warnings) - # 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) - params = {'category': cat_name} - elif import_method == ROUND_METHOD: + if import_method == ROUND_METHOD: threshold = request_dict['threshold'] prev_round_id = request_dict['previous_round_id'] entries = coord_dao.get_rating_advancing_group(prev_round_id, threshold) params = {'threshold': threshold, 'round_id': prev_round_id} - elif import_method == SELECTED_METHOD: - file_names = request_dict['file_names'] - entries, warnings = coord_dao.add_entries_by_name(round_id, file_names) - if warnings: - formatted_warnings = u'\n'.join([ - 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}) - params = {'file_names': file_names} + elif import_method in import_check.CHECKED_METHODS: + token = request_dict.get('check_token') + if not token: + raise ImportCheckRequired( + 'check the import source first: POST it to' + ' /admin/campaign//import/check, then import' + ' with the check_token it returns') + result = import_check.load(token, config, coord_dao.campaign.id, + import_method) + import_check.raise_if_blocked(result) # before anything is written + entries = [make_entry(row['commons']) + for row in import_check.importable(result)] + entries = coord_dao.add_checked_entries( + round_id, entries, import_check.describe_source(result)) + params = dict(import_check.source_params(result), check_token=token) + import_warnings.extend(import_check.import_warnings(result)) else: raise NotImplementedResponse() @@ -391,7 +427,8 @@ def import_entries(user_dao, round_id, request_dict): # automatically disqualify entries based on round config auto_dq = autodisqualify(user_dao, round_id, request_dict={}) new_entry_stats['disqualified'] = auto_dq['data'] - if len(new_entry_stats['disqualified']) >= len(entries): + # (an empty import is not "all disqualified", #208) + if entries and len(new_entry_stats['disqualified']) >= len(entries): new_entry_stats['warnings'].append({'all disqualified': 'all entries disqualified by round settings'}) diff --git a/montage/app.py b/montage/app.py index 3219be11..affb95e7 100644 --- a/montage/app.py +++ b/montage/app.py @@ -25,6 +25,7 @@ SQLProfilerMiddleware) from .rdb import Base, bootstrap_maintainers, ensure_series from .utils import get_env_name, load_env_config +from . import import_check from .check_rdb import get_schema_errors, ping_connection from .meta_endpoints import META_API_ROUTES, META_UI_ROUTES @@ -67,6 +68,12 @@ def create_app(env_name='prod', config=None): config = load_env_config(env_name=env_name) print('== loaded config file: %s' % (config['__file__'],)) + # import check files (#510) must be on storage all pods share + check_dir_problem = import_check.check_dir_problem(config, env_name) + if check_dir_problem: + raise ValueError(check_dir_problem) + print('== import check folder: %s' % (import_check.check_dir(config),)) + engine = create_engine(config.get('db_url', DEFAULT_DB_URL), pool_recycle=60) session_type = sessionmaker() session_type.configure(bind=engine) diff --git a/montage/rdb.py b/montage/rdb.py index f0894377..be4004d8 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -654,12 +654,6 @@ def _entry_insert_row(entry): return {key: getattr(entry, key) for key in _ENTRY_INSERT_COLS} -# Imports have stored Commons' file_id since 2026-05-31: on production the -# last category-imported entry without one dates from 2026-05-30 13:21, the -# first with one from 2026-05-31 20:52. A file new to Montage after this date -# therefore has its file_id stored (hatnote/montage#513). -FILE_ID_LOOKUP_CUTOFF = datetime.datetime(2026, 6, 1) - # Batch size for lookups by file_id: below 200 on purpose. MariaDB's # eq_range_index_dive_limit is 200 by default (ToolsDB too): with fewer # values in an IN (...) list the optimizer counts the matching rows in the @@ -675,21 +669,17 @@ def _entry_insert_row(entry): FILE_ID_LOOKUP_CHUNK_SIZE = 199 -def _use_file_id_lookup(rnd, entries): +def _use_file_id_lookup(entries): """Whether add_entries looks entries up by file_id instead of by name - (#513): when the campaign opened after FILE_ID_LOOKUP_CUTOFF and every - entry has a file_id (wikireplica imports; CSV imports have none). - Production has no index on entries.name, so a lookup by name reads the - whole table; by file_id it uses ix_entries_file_id, in batches of - FILE_ID_LOOKUP_CHUNK_SIZE. + (#513): when every entry has a file_id, which every checked import + gives (#510), in every campaign. Production has no index on + entries.name, so a lookup by name reads the whole table; by file_id it + uses ix_entries_file_id, in batches of FILE_ID_LOOKUP_CHUNK_SIZE. Files not found by file_id are looked up by name only where entries.name is indexed (see _add_entries_by_file_id); elsewhere a file imported - before the cutoff, without a file_id, gets a second entries row. - Campaigns opening after the cutoff rarely contain such files.""" - open_date = rnd.campaign.open_date if rnd.campaign else None - if open_date is None or open_date < FILE_ID_LOOKUP_CUTOFF: - return False + before 2026-05-31, stored without a file_id, gets a second entries row + (accepted until the backfill, #513).""" return bool(entries) and all(e.file_id is not None for e in entries) @@ -1658,58 +1648,19 @@ def activate_round(self, round_id): return - def add_entries_from_cat(self, round_id, cat_name): + def add_checked_entries(self, round_id, entries, source_description): + """Store the entries of a checked import (import_check.py, #510); + returns the stored Entry rows, in the order given.""" rnd = self.user_dao.get_round(round_id) - if ENV_NAME == 'dev': - source = 'remote' - else: - source = 'local' - entries = loaders.load_category(cat_name, source=source) entries, new_entry_count = self.add_entries(rnd, entries) - msg = ('%s loaded %s entries from category (%s), %s new entries added' - % (self.user.username, len(entries), cat_name, new_entry_count)) + msg = ('%s loaded %s entries from %s, %s new entries added' + % (self.user.username, len(entries), source_description, + new_entry_count)) self.log_action('add_entries', message=msg, round=rnd) return entries - def add_entries_by_name(self, round_id, file_names): - rnd = self.user_dao.get_round(round_id) - if ENV_NAME == 'dev': - source = 'remote' - else: - source = 'local' - entries, warnings = loaders.load_by_filename(file_names, source=source) - entries, new_entry_count = self.add_entries(rnd, entries) - - msg = ('%s loaded %s entries from filenames, %s new entries added' - % (self.user.username, len(entries), new_entry_count)) - self.log_action('add_entries', message=msg, round=rnd) - - return entries, warnings - - def add_entries_from_csv(self, round_id, csv_url): - # NOTE: this no longer creates RoundEntries, use - # add_round_entries to do this. - rnd = self.user_dao.get_round(round_id) - if ENV_NAME == 'dev': - source = 'remote' - else: - source = 'local' - try: - entries, warnings = loaders.get_entries_from_csv(csv_url, - source=source) - except ValueError: - raise InvalidAction('unable to load csv "%s"' % csv_url) - - entries, new_entry_count = self.add_entries(rnd, entries) - - msg = ('%s loaded %s entries from csv (%r), %s new entries added' - % (self.user.username, len(entries), csv_url, new_entry_count)) - self.log_action('add_entries', message=msg, round=rnd) - - return entries, warnings - def get_round_sources(self, round_id, import_method): round_sources = (self.query(RoundSource) .filter_by(round_id=round_id, @@ -1750,7 +1701,7 @@ def add_entries(self, rnd, entries): deduped.append(e) entries = deduped - if _use_file_id_lookup(rnd, entries): + if _use_file_id_lookup(entries): return self._add_entries_by_file_id(entries) entry_chunks = chunked(entries, IMPORT_CHUNK_SIZE) diff --git a/montage/tests/test_import_entries.py b/montage/tests/test_import_entries.py index d9a4f22f..12a173bc 100644 --- a/montage/tests/test_import_entries.py +++ b/montage/tests/test_import_entries.py @@ -189,11 +189,11 @@ def new_round(api_client, name, config=None, quorum=None, open_date=OPEN_DATE, def import_category(api_client, round_id, **kw): - return api_client.fetch('coordinator: import category', - '/admin/round/%s/import' % round_id, - {'import_method': 'category', - 'category': 'Synthetic_test_category'}, - as_user=COORD, **kw) + return api_client.fetch_checked_import('coordinator: import category', + round_id, + {'import_method': 'category', + 'category': 'Synthetic_test_category'}, + as_user=COORD, **kw) def _load_json(raw): @@ -226,6 +226,8 @@ def round_rows(app, round_id, strip_prefix=''): row['flags'] = _load_json(row['flags']) row['re_flags'] = _load_json(row['re_flags']) row['source_params'] = _load_json(row['source_params']) + # random per check (#510); stored, see test_check_token_is_stored... + row['source_params'].pop('check_token', None) row['name'] = row['name'][len(strip_prefix):] if row['flags'].get('archive_name'): row['flags']['archive_name'] = row['flags']['archive_name'].replace(strip_prefix, '') @@ -481,10 +483,11 @@ def test_mysql_driver_batches_the_inserts(): # --------------------------------------------------------------------------- -# Lookup by file_id for campaigns after the cutoff (#513) +# Lookup by file_id (#513, #654); since #510 in every campaign, not only in +# campaigns opened from 2026-06-01 (the former FILE_ID_LOOKUP_CUTOFF) # --------------------------------------------------------------------------- -AFTER_CUTOFF = rdb.FILE_ID_LOOKUP_CUTOFF + datetime.timedelta(days=30) +AFTER_CUTOFF = datetime.datetime(2026, 6, 1) + datetime.timedelta(days=30) def with_file_ids(infos): @@ -538,12 +541,15 @@ def test_file_id_lookup_after_cutoff(montage_app, coord_client, assert entry_ids(montage_app, 'fid') == ids_after -def test_name_lookup_before_cutoff(montage_app, coord_client, - mock_external_apis, monkeypatch): +def test_file_id_lookup_in_old_campaigns_too(montage_app, coord_client, + mock_external_apis, monkeypatch): + """#510: checked imports match by file_id whatever the campaign's open + date; #654 did so only from 2026-06-01.""" infos = with_file_ids(make_file_infos('old', 40)) round_id = new_round(coord_client, 'old') # opens in 2015 - monkeypatch.setattr(CoordinatorDAO, 'get_entry_file_id_map', - _no_file_id_lookup) + monkeypatch.setattr(rdb, '_entries_indexes', lambda session: ( + {'PRIMARY', 'ix_entries_file_id'}, {'id', 'file_id'})) + monkeypatch.setattr(CoordinatorDAO, 'get_entry_name_map', _no_name_lookup) mock_category(mock_external_apis, infos) import_category(coord_client, round_id) assert len(entry_ids(montage_app, 'old')) == 40 diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index eb7bd913..1a8516bb 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -118,6 +118,20 @@ def fetch(self, role_action, url, data=None, **kw): raise return data_dict + def fetch_checked_import(self, role_action, round_id, data, **kw): + """Import into a first round the way the round form does (#510): + check the source for the round's campaign, then import with the + check token. Returns the import's response.""" + as_user = kw.get('as_user') + rnd = self.fetch('coordinator: get round', '/admin/round/%s' % round_id, + as_user=as_user)['data'] + check = self.fetch('coordinator: check import source', + '/admin/campaign/%s/import/check' % rnd['campaign']['id'], + data, as_user=as_user)['data'] + return self.fetch(role_action, '/admin/round/%s/import' % round_id, + {'import_method': data['import_method'], + 'check_token': check['token']}, **kw) + def _create_schema(db_url, echo=True): from sqlalchemy import create_engine @@ -133,6 +147,7 @@ def _create_schema(db_url, echo=True): def montage_app(tmpdir): config = utils.load_env_config(env_name='devtest') config['db_url'] = config['db_url'].replace('///', '///' + str(tmpdir) + '/') + config['import_check_path'] = str(tmpdir.join('import_checks')) db_url = config['db_url'] _create_schema(db_url=db_url) @@ -318,8 +333,8 @@ def test_home_client(base_client, api_client, mock_external_apis): data = {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'} - resp = fetch('coordinator: import entries from a category', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import entries from a category', + round_id, data, as_user='LilyOfTheWest') resp = fetch('coordinator: activate a round', @@ -333,13 +348,13 @@ def test_home_client(base_client, api_client, mock_external_apis): as_user='LilyOfTheWest') gsheet_url = 'https://docs.google.com/spreadsheets/d/1WzHFg_bhvNthRMwNmxnk010KJ8fwuyCrby29MvHUzH8/edit#gid=550467819' - resp = fetch('coordinator: import more entries from different gsheet csv into an existing round', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import more entries from different gsheet csv into an existing round', + round_id, {'import_method': 'csv', 'csv_url': gsheet_url}, as_user='LilyOfTheWest') - resp = fetch('coordinator: import files selected by name', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import files selected by name', + round_id, {'import_method': 'selected', 'file_names': ['Reynisfjara, Suðurland, Islandia, 2014-08-17, DD 164.JPG']}, as_user='LilyOfTheWest') @@ -921,9 +936,9 @@ def test_import_entries_have_file_id(api_client, mock_external_apis): ) round_id = rnd_resp['data']['id'] - api_client.fetch( + api_client.fetch_checked_import( 'coordinator: import entries via category', - '/admin/round/%s/import' % round_id, + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='Yarl', @@ -1013,9 +1028,9 @@ def count_inserts(conn, cursor, statement, params, context, executemany): inserts[table] = inserts.get(table, 0) + 1 def do_import(): - return api_client.fetch( + return api_client.fetch_checked_import( 'coordinator: import entries via category', - '/admin/round/%s/import' % round_id, + round_id, {'import_method': 'category', 'category': 'Batch_import'}, as_user='Yarl')['data'] @@ -1175,8 +1190,8 @@ def test_vote_later_reappears(api_client, mock_external_apis): as_user='LilyOfTheWest') round_id = resp['data']['id'] - fetch('coordinator: import entries', - '/admin/round/%s/import' % round_id, + api_client.fetch_checked_import('coordinator: import entries', + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='LilyOfTheWest') @@ -1273,8 +1288,8 @@ def test_concurrent_activation_creates_no_duplicate_tasks(montage_app, api_clien as_user='LilyOfTheWest') round_id = resp['data']['id'] - fetch('coordinator: import entries', - '/admin/round/%s/import' % round_id, + api_client.fetch_checked_import('coordinator: import entries', + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='LilyOfTheWest') @@ -1357,8 +1372,8 @@ def test_activation_inserts_tasks_in_batches(montage_app, api_client, as_user='LilyOfTheWest') round_id = resp['data']['id'] - resp = fetch('coordinator: import entries', - '/admin/round/%s/import' % round_id, + resp = api_client.fetch_checked_import('coordinator: import entries', + round_id, {'import_method': 'category', 'category': 'Images_from_Wiki_Loves_Monuments_2015_in_Albania'}, as_user='LilyOfTheWest') @@ -1549,8 +1564,8 @@ def test_selected_import_with_missing_name_reports_warning(montage_app, api_clie 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']}, as_user='Yarl')['data']['id'] - data = api_client.fetch( - 'coordinator: import selected files', '/admin/round/%s/import' % round_id, + data = api_client.fetch_checked_import( + 'coordinator: import selected files', round_id, {'import_method': 'selected', 'file_names': [SELECTED_FILE_INFO['img_name'], 'Not on Commons.jpg']}, as_user='Yarl')['data'] @@ -1563,13 +1578,13 @@ def test_selected_import_with_missing_name_reports_warning(montage_app, api_clie 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 + import responses as responses_lib + from montage.tests.conftest import FIXTURE_FILE_INFOS, build_filename_csv - 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) + # a plain CSV link (fetched as given, #208): 3 known names + 1 unknown + mock_external_apis.add(responses_lib.GET, 'https://example.org/files.csv', + body=build_filename_csv(FIXTURE_FILE_INFOS[:3]) + 'Gone.jpg\n', + 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'] @@ -1585,8 +1600,8 @@ def fake_csv(url, source='local'): 'jurors': ['Slaporte', 'MahmoudHashemi', 'Effeietsanders']}, as_user='Yarl')['data']['id'] - data = api_client.fetch( - 'coordinator: import csv', '/admin/round/%s/import' % round_id, + data = api_client.fetch_checked_import( + 'coordinator: import csv', 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) diff --git a/tools/admin.py b/tools/admin.py index 42dfb8a0..9a9824a6 100644 --- a/tools/admin.py +++ b/tools/admin.py @@ -38,6 +38,10 @@ CoordinatorDAO, reassign_rating_tasks, lookup_user) +from montage.import_check import (run_check, raise_if_blocked, importable, + import_warnings, describe_source, + source_params, lookup_source) +from montage.loaders import make_entry from montage.utils import get_threshold_map @@ -218,14 +222,12 @@ def create_round(user_dao, campaign_id, advance=False, debug=False): campaign = user_dao.get_campaign(campaign_id) if not advance: - entries = coord_dao.add_entries_from_cat(rnd.id, category_name) - source = category_name - # GIST_URL = 'https://gist.githubusercontent.com/slaporte/7433943491098d770a8e9c41252e5424/raw/ca394147a841ea5f238502ffd07cbba54b9b1a6a/wlm2015_fr_500.csv' - # entries = maint_dao.add_entries_from_csv_gist(rnd, GIST_URL) - # source = GIST_URL + request = {'import_method': 'category', 'category': category_name} + entries, params = _checked_entries(user_dao, coord_dao, rnd.id, request) print(('++ prepared %s entries from %r' % - (len(entries), source))) - coord_dao.add_round_entries(rnd.id, entries) + (len(entries), category_name))) + coord_dao.add_round_entries(rnd.id, entries, method='category', + params=params) else: final_rnds = [r for r in campaign.rounds if r.status == 'finalized'] last_successful_rnd = final_rnds[-1] @@ -466,13 +468,29 @@ def check_round_dupes(user_dao, round_id): return +def _checked_entries(user_dao, coord_dao, round_id, request): + """Check an import source like the round form does (#510), stop on + blocking rows, and store the checked entries; returns (entries, + round source params). The check is not written to a file.""" + result = run_check(request, coord_dao.campaign.id, source=lookup_source(), + rdb_session=user_dao.rdb_session) + raise_if_blocked(result) + for warning in import_warnings(result): + print('!! %s' % (list(warning.values())[0],)) + entries = [make_entry(row['commons']) for row in importable(result)] + entries = coord_dao.add_checked_entries(round_id, entries, + describe_source(result)) + return entries, source_params(result) + + def import_gist(user_dao, round_id, url): "import round entries from a csv list" coord_dao = CoordinatorDAO.from_round(user_dao, round_id) - entries, warnings = coord_dao.add_entries_from_csv(round_id, url) + request = {'import_method': 'gistcsv', 'gist_url': url} + entries, params = _checked_entries(user_dao, coord_dao, round_id, request) stats = coord_dao.add_round_entries(round_id, entries, method='gistcsv', - params={'gist_url': url}) + params=params) print('++ added entries to round %s: %r' % (round_id, stats)) From fbc1255646a976190a8183ef5fc0bf11c6799281 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:31:19 +0000 Subject: [PATCH 04/20] wip(#510): endpoint tests (check, download, token import, retry, expiry) Co-Authored-By: Claude Opus 5.5 --- montage/tests/test_import_check.py | 245 +++++++++++++++++++++++++++++ 1 file changed, 245 insertions(+) diff --git a/montage/tests/test_import_check.py b/montage/tests/test_import_check.py index 2b52a7d1..3a38693a 100644 --- a/montage/tests/test_import_check.py +++ b/montage/tests/test_import_check.py @@ -414,3 +414,248 @@ def test_header_less_list_with_comma_names(monkeypatch, commons): result = csv_check(monkeypatch, u'%s\n%s\n' % (VOLOCHEK.replace('_', ' '), VOLOCHYOK)) assert [(r['row'], r['commons_name'], r['status']) for r in result['rows']] == [ (1, VOLOCHEK, 'same_name'), (2, VOLOCHYOK, 'same_name')] + + +# --------------------------------------------------------------------------- +# Endpoints: check, download, import by token +# --------------------------------------------------------------------------- + +from montage.tests.test_web_basic import montage_app, api_client # noqa: E402,F401 +from montage.tests.test_import_entries import ( # noqa: E402,F401 + COORD, coord_client, new_round, db_query, entry_ids, round_rows) + + +def _campaign_of(client, round_id): + return client.fetch('coordinator: get round', '/admin/round/%s' % round_id, + as_user=COORD)['data']['campaign']['id'] + + +@pytest.fixture +def local_commons(monkeypatch, commons): + """Endpoint tests run as 'dev'; look Commons up 'locally' (stand-in) + so lookups by file_id happen, as on Toolforge.""" + monkeypatch.setattr(import_check, 'lookup_source', lambda: 'local') + return commons + + +def _source(monkeypatch, text): + monkeypatch.setattr(import_check, 'fetch_source_text', lambda url: text) + + +def _check(client, campaign_id, error_code=None, **request): + request.setdefault('import_method', 'csv') + request.setdefault('csv_url', 'https://example.org/list.csv') + kw = {'error_code': error_code} if error_code else {} + return client.fetch('coordinator: check import source', + '/admin/campaign/%s/import/check' % campaign_id, + request, as_user=COORD, **kw) + + +def _import(client, round_id, token, method='csv', error_code=None): + data = {'import_method': method} + if token: + data['check_token'] = token + kw = {'error_code': error_code} if error_code else {} + return client.fetch('coordinator: import', '/admin/round/%s/import' % round_id, + data, as_user=COORD, **kw) + + +def _body(resp): + return resp.get_data(as_text=True) + + +def test_check_endpoint_writes_no_rows(montage_app, coord_client, local_commons, + monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'filename,File ID\nA.jpg,x\nGone.jpg,\n') + round_id = new_round(coord_client, 'chk') + data = _check(coord_client, _campaign_of(coord_client, round_id))['data'] + assert len(data['token']) == 32 + assert data['counts']['ok'] == 1 and data['counts']['unknown_name'] == 1 + assert data['columns']['ignored'] == ['File ID'] + assert not data['blocking'] and data['importable_count'] == 1 + assert [i['row'] for i in data['issues']] == [3] + assert db_query(montage_app, 'SELECT id FROM entries') == [] + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + + +def test_check_and_download_need_a_coordinator(montage_app, coord_client, + local_commons, monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'perm') + campaign_id = _campaign_of(coord_client, round_id) + token = _check(coord_client, campaign_id)['data']['token'] + coord_client.fetch('maintainer: add organizer', '/admin/add_organizer', + {'username': 'Outsider'}) + coord_client.fetch('outsider: check', '/admin/campaign/%s/import/check' % campaign_id, + {'import_method': 'csv', 'csv_url': 'x'}, as_user='Outsider', + error_code=403) + coord_client.fetch('outsider: download', + '/admin/campaign/%s/import/check/%s/download' % (campaign_id, token), + as_user='Outsider', error_code=403) + + +def test_download(montage_app, coord_client, local_commons, monkeypatch): + local_commons([info('A.jpg', 1), info('B_new.jpg', 2)]) + _source(monkeypatch, 'filename,file_id\nA.jpg,1\nB old.jpg,2\nGone.jpg,\n') + round_id = new_round(coord_client, 'dl') + campaign_id = _campaign_of(coord_client, round_id) + token = _check(coord_client, campaign_id)['data']['token'] + resp = coord_client.fetch('coordinator: download check', + '/admin/campaign/%s/import/check/%s/download' + % (campaign_id, token), as_user=COORD) + assert resp.headers['Content-Disposition'].startswith( + 'attachment; filename=montage_import-%s-' % campaign_id) + assert _body(resp) == 'filename,file_id\nA.jpg,1\nB_new.jpg,2\n' + + +def test_import_without_a_token_is_refused(montage_app, coord_client): + round_id = new_round(coord_client, 'notoken') + for method in ('csv', 'gistcsv', 'category', 'selected'): + resp = _import(coord_client, round_id, None, method, error_code=400) + assert 'import_check_required' in _body(resp) + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + + +def test_import_takes_the_checked_list_without_lookups(montage_app, coord_client, + local_commons, monkeypatch): + """#509 + #510: a full CSV with stale metadata imports Commons' data and + file_ids; the import fetches nothing and asks Commons nothing.""" + local_commons([info('A.jpg', 1, img_width=4000), info('B_new.jpg', 2)]) + _source(monkeypatch, 'img_name,file_id,img_width,img_user_text\n' + 'A.jpg,,10,Stale\nB old.jpg,2,10,Stale\n') + round_id = new_round(coord_client, 'bytoken') + token = _check(coord_client, _campaign_of(coord_client, round_id))['data']['token'] + + def boom(*a, **kw): + raise AssertionError('looked something up during the import') + for name in ('fetch_source_text', 'lookup_by_names', 'lookup_by_ids', + 'category_records'): + monkeypatch.setattr(import_check, name, boom) + from montage import labs + monkeypatch.setattr(labs, '_connect', boom) + with responses_lib.RequestsMock(): # any HTTP request fails + data = _import(coord_client, round_id, token)['data'] + + assert data['new_round_entry_count'] == 2 + assert ['renamed'] in [list(w) for w in data['warnings']] + assert all(len(w) == 1 for w in data['warnings']) # one-key dicts + rows = round_rows(montage_app, round_id) + assert [(r['name'], r['file_id'], r['width'], r['upload_user_text']) + for r in rows] == [('A.jpg', 1, 4000, 'Uploader'), + ('B_new.jpg', 2, 3000, 'Uploader')] + assert rows[0]['source_params']['csv_url'] == 'https://example.org/list.csv' + + +def test_check_token_is_stored_in_the_round_source(montage_app, coord_client, + local_commons, monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'param') + token = _check(coord_client, _campaign_of(coord_client, round_id))['data']['token'] + _import(coord_client, round_id, token) + params = db_query(montage_app, 'SELECT params FROM round_sources WHERE round_id = :r', + r=round_id) + assert json.loads(params[0]['params']) == { + 'csv_url': 'https://example.org/list.csv', 'check_token': token} + + +def test_blocked_check_writes_nothing_and_retry_uses_the_same_round( + montage_app, coord_client, local_commons, monkeypatch): + files = [info('F%03d.jpg' % i, 1000 + i) for i in range(100)] + local_commons(files) + rows = ''.join('F%03d.jpg,%d\n' % (i, 1000 + i) for i in range(100)) + _source(monkeypatch, 'filename,file_id\n' + rows.replace(',1050\n', ',9999\n')) + round_id = new_round(coord_client, 'blocked') + campaign_id = _campaign_of(coord_client, round_id) + check = _check(coord_client, campaign_id)['data'] + assert check['blocking'] and check['counts']['unknown_file_id'] == 1 + + resp = _import(coord_client, round_id, check['token'], error_code=400) + assert 'import_check_blocked' in _body(resp) + assert 'row 52: file_id 9999 is not on Commons' in _body(resp) + assert db_query(montage_app, 'SELECT id FROM entries') == [] + assert round_rows(montage_app, round_id) == [] + + # the coordinator fixes the source, checks again, imports into the + # round that already exists (the form does not create a second one) + _source(monkeypatch, 'filename,file_id\n' + rows) + check = _check(coord_client, campaign_id)['data'] + data = _import(coord_client, round_id, check['token'])['data'] + assert data['new_round_entry_count'] == 100 + rounds = coord_client.fetch('coordinator: campaign', '/admin/campaign/%s' % campaign_id, + as_user=COORD)['data']['rounds'] + assert len(rounds) == 1 + + +def test_bad_tokens_via_the_endpoint(montage_app, coord_client, local_commons, + monkeypatch): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_a = new_round(coord_client, 'tok a') + round_b = new_round(coord_client, 'tok b') + token_b = _check(coord_client, _campaign_of(coord_client, round_b))['data']['token'] + for token, method in [('../../etc/passwd', 'csv'), ('x' * 32, 'csv'), + (token_b, 'csv'), # another campaign + ]: + resp = _import(coord_client, round_a, token, method, error_code=400) + assert 'import_check_expired' in _body(resp) + resp = _import(coord_client, round_b, token_b, 'category', error_code=400) + assert 'import_check_expired' in _body(resp) + assert db_query(montage_app, 'SELECT id FROM round_sources') == [] + + +def test_import_after_seven_days_is_refused(montage_app, coord_client, + local_commons, monkeypatch, tmpdir): + local_commons([info('A.jpg', 1)]) + _source(monkeypatch, 'A.jpg\n') + round_id = new_round(coord_client, 'old check') + token = _check(coord_client, _campaign_of(coord_client, round_id))['data']['token'] + later = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(days=8) + monkeypatch.setattr(import_check, '_utcnow', lambda: later) + resp = _import(coord_client, round_id, token, error_code=400) + assert 'import_check_expired' in _body(resp) and '7 days' in _body(resp) + + +def test_renamed_file_already_in_montage_keeps_its_row(montage_app, coord_client, + local_commons, monkeypatch): + """Review fix 1: a file Montage already has (found by file_id) keeps its + row and stored name; only a file new to Montage gets Commons' name.""" + local_commons([info('Old_name.jpg', 5)]) + _source(monkeypatch, 'Old_name.jpg\n') + round_a = new_round(coord_client, 'ren a') + _import(coord_client, round_a, _check(coord_client, _campaign_of(coord_client, round_a))['data']['token']) + ids = entry_ids(montage_app, 'Old_name') + + local_commons([info('New_name.jpg', 5)]) + _source(monkeypatch, 'filename,file_id\nOld name.jpg,5\n') + round_b = new_round(coord_client, 'ren b') + check = _check(coord_client, _campaign_of(coord_client, round_b))['data'] + assert check['counts']['renamed'] == 1 + data = _import(coord_client, round_b, check['token'])['data'] + assert data['new_round_entry_count'] == 1 + assert entry_ids(montage_app, 'Old_name') == ids + assert entry_ids(montage_app, 'New_name') == {} + + +def test_category_same_name_pair_keeps_the_first(montage_app, coord_client, + local_commons): + local_commons([info(VOLOCHEK, 149673020), info(VOLOCHYOK, 149673022)]) + round_id = new_round(coord_client, 'cat pair') + check = _check(coord_client, _campaign_of(coord_client, round_id), + import_method='category', category='Cat')['data'] + assert not check['blocking'] and check['importable_count'] == 1 + data = _import(coord_client, round_id, check['token'], 'category')['data'] + assert data['new_round_entry_count'] == 1 + same = [w['same name'] for w in data['warnings'] if 'same name' in w] + assert len(same) == 1 and VOLOCHEK in same[0] and VOLOCHYOK in same[0] + + +def test_app_refuses_to_start_without_a_check_folder(monkeypatch): + from montage.app import create_app + monkeypatch.delenv('MONTAGE_IMPORT_CHECK_PATH', raising=False) + with pytest.raises(ValueError) as exc: + create_app('prod', config={'__file__': 'test', '__env__': 'prod', + 'db_url': 'sqlite://'}) + assert 'MONTAGE_IMPORT_CHECK_PATH' in str(exc.value) From 8046d37a05555de6483170fefc1f35d9ddde6e21 Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Mon, 5 Oct 2026 19:34:09 +0000 Subject: [PATCH 05/20] wip(#510): round form checks the source; round created once - Check button + ImportCheckResult panel (text only), download link - Save only after a non-blocking check; source change clears the check; stale check responses ignored; source locked while checking - the round is created once; a failed import is retried into it, with the round settings locked - i18n (en + qqq) for the new texts and the import warning dialog - Cypress: check before Add Round (not run) Refs hatnote/montage#510 Co-Authored-By: Claude Opus 5.5 --- frontend/cypress/e2e/campaign.cy.js | 31 ++- .../components/Round/ImportCheckResult.vue | 157 ++++++++++++ frontend/src/components/Round/RoundNew.vue | 224 ++++++++++++++---- frontend/src/i18n/en.json | 33 ++- frontend/src/i18n/qqq.json | 33 ++- frontend/src/services/adminService.js | 7 +- 6 files changed, 438 insertions(+), 47 deletions(-) create mode 100644 frontend/src/components/Round/ImportCheckResult.vue diff --git a/frontend/cypress/e2e/campaign.cy.js b/frontend/cypress/e2e/campaign.cy.js index bbfcf20e..69c346bb 100644 --- a/frontend/cypress/e2e/campaign.cy.js +++ b/frontend/cypress/e2e/campaign.cy.js @@ -84,7 +84,36 @@ describe('Campaign Details Page', () => { .find('li') .first() .click(); - cy.get('.button-group button').contains('Add Round').click().click(); + // #510: the source is checked before the round can be saved + cy.intercept('POST', '/v1/admin/campaign/*/import/check', { + body: { + status: 'success', + data: { + token: 'cypress-check-token-000000000000', + checked_at: '2025-08-01T00:00:00Z', + expires_at: '2025-08-08T00:00:00Z', + import_method: 'selected', + source: { file_names: ['Example.jpg'] }, + columns: { name: null, file_id: null, ignored: [] }, + counts: { ok: 1, renamed: 0, duplicate: 0, unknown_name: 0, + unknown_file_id: 0, malformed_file_id: 0, same_name: 0 }, + blocking: false, + total_rows: 1, + importable_count: 1, + issues: [], + issues_total: 0, + issues_truncated: false, + same_name_groups: [] + } + } + }).as('checkImport') + cy.get('.form-container').contains('label', 'File List').click() + cy.get('.form-container textarea').first().type('Example.jpg') + cy.get('.button-group button').contains('Add Round').should('be.disabled') + cy.get('[data-testid="check-source-button"]').click() + cy.wait('@checkImport') + cy.get('[data-testid="import-check-result"]').should('be.visible') + cy.get('.button-group button').contains('Add Round').click(); cy.log(' Round created successfully'); }) diff --git a/frontend/src/components/Round/ImportCheckResult.vue b/frontend/src/components/Round/ImportCheckResult.vue new file mode 100644 index 00000000..ff2f646e --- /dev/null +++ b/frontend/src/components/Round/ImportCheckResult.vue @@ -0,0 +1,157 @@ + + + + + diff --git a/frontend/src/components/Round/RoundNew.vue b/frontend/src/components/Round/RoundNew.vue index b3e4d129..a5fcd581 100644 --- a/frontend/src/components/Round/RoundNew.vue +++ b/frontend/src/components/Round/RoundNew.vue @@ -33,14 +33,18 @@ - + {{ $t('montage-round-no-category') }} - + - + - +
+ + {{ + isChecking + ? $t('montage-round-check-running') + : $t('montage-round-check-source') + }} + +

+ {{ $t('montage-round-check-required') }} +

+ +
+ - + {{ $t('montage-description-round-stats') }}

 - + - +

{{ $t('montage-round-file-setting') }}

- + - +
diff --git a/frontend/src/i18n/en.json b/frontend/src/i18n/en.json index 0da79d79..a8ae6a1e 100644 --- a/frontend/src/i18n/en.json +++ b/frontend/src/i18n/en.json @@ -214,6 +214,9 @@ "montage-round-check-status-unknown-name": "Not on Commons", "montage-round-check-status-unknown-file-id": "File ID not on Commons", "montage-round-check-status-malformed-file-id": "Not a valid file ID", + "montage-round-check-status-page-id": "Page ID, not a file ID", + "montage-round-check-status-revision-id": "Revision ID, not a file ID", + "montage-round-check-status-ambiguous-id": "File ID that is also a page or revision ID", "montage-round-check-status-same-name": "Same name as another file inside this import", "montage-round-check-same-name-title": "Different files with names the database treats as the same", "montage-round-check-same-name-list": "Rename one of them on Commons or remove one row from the source, then check again.", diff --git a/frontend/src/i18n/qqq.json b/frontend/src/i18n/qqq.json index e12086c1..0d010009 100644 --- a/frontend/src/i18n/qqq.json +++ b/frontend/src/i18n/qqq.json @@ -214,6 +214,9 @@ "montage-round-check-status-unknown-name": "Import check status: no file with this name exists on Commons; the row is left out.", "montage-round-check-status-unknown-file-id": "Import check status: no file with this file ID exists on Commons; this blocks the import.", "montage-round-check-status-malformed-file-id": "Import check status: the file ID is not a plain whole number (for example 1.49673E+08); this blocks the import.", + "montage-round-check-status-page-id": "Import check status: a number in the list is the page ID of a file page on Commons (shown under Page information), not the file ID Montage needs; this blocks the import.", + "montage-round-check-status-revision-id": "Import check status: a number in the list is a revision ID (one edit of a file page on Commons), not the file ID Montage needs; this blocks the import.", + "montage-round-check-status-ambiguous-id": "Import check status: a number in the list is the file ID of one file but also the page ID or a revision ID of another file page, so it is unclear which file is meant; this blocks the import.", "montage-round-check-status-same-name": "Import check status: a different file in the same import (not a file already in Montage) has a name that the database treats as the same (for example differing only in case, accents or е/ё).", "montage-round-check-same-name-title": "Heading above pairs of file names that the database cannot tell apart.", "montage-round-check-same-name-list": "Advice for same-name pairs in a CSV or file list (they block the import).", diff --git a/montage/docs/api.md b/montage/docs/api.md index bf0b1d38..7988fd28 100644 --- a/montage/docs/api.md +++ b/montage/docs/api.md @@ -263,7 +263,7 @@ its downloads stay at most 7 days. Writes nothing to the database. ### Response - `data`: `token`, `checked_at`, `expires_at`, `import_method`, `source`, `columns`, `counts` (per status: `ok`, `renamed`, `duplicate`, `unknown_name`, `unknown_file_id`, - `malformed_file_id`, `same_name`), `blocking`, `total_rows`, `importable_count`, `issues` (rows + `malformed_file_id`, `page_id`, `revision_id`, `ambiguous_id`, `same_name`), `blocking`, `total_rows`, `importable_count`, `issues` (rows that are not ok, at most 1000), `issues_total`, `issues_truncated`, `same_name_groups` ### Errors diff --git a/montage/import_check.py b/montage/import_check.py index 37fa7593..ad333bfb 100644 --- a/montage/import_check.py +++ b/montage/import_check.py @@ -17,6 +17,11 @@ warning unknown_file_id the file_id is not on Commons: blocks malformed_file_id the file_id is not a plain whole number: blocks + page_id a bare number (no name) that is a File: page's page ID, + not a file_id: blocks + revision_id a bare number that is a revision ID of a File: page: blocks + ambiguous_id a bare number that is one file's file_id and another + File: page's page or revision ID: blocks same_name a different Commons file whose name the database treats as equal to another's (utf8mb4_unicode_ci): blocks a list import (CSV, gist, Sheet, file list); in a category @@ -43,7 +48,8 @@ from .loaders import (fetch_source_text, parse_source_rows, parse_name_list, pasted_csv_text, - lookup_by_names, lookup_by_ids, category_records, + lookup_by_names, lookup_by_ids, lookup_other_ids, + category_records, guard_cell) from .utils import (PROJ_PATH, get_env_name, ImportSourceInvalid, ImportCheckBlocked, ImportCheckExpired) @@ -53,10 +59,12 @@ CHECKED_METHODS = ('csv', 'gistcsv', 'category', 'selected') LIST_METHODS = ('csv', 'gistcsv', 'selected') # same-name pairs block these -BLOCKING_STATUSES = ('unknown_file_id', 'malformed_file_id', 'same_name') +BLOCKING_STATUSES = ('unknown_file_id', 'malformed_file_id', 'page_id', + 'revision_id', 'ambiguous_id', 'same_name') IMPORTED_STATUSES = ('ok', 'renamed') ALL_STATUSES = ('ok', 'renamed', 'duplicate', 'unknown_name', - 'unknown_file_id', 'malformed_file_id', 'same_name') + 'unknown_file_id', 'malformed_file_id', 'page_id', 'revision_id', + 'ambiguous_id', 'same_name') # A check can be imported for an hour: files are renamed and deleted on # Commons all the time, and the check and the save happen in one sitting. @@ -185,10 +193,18 @@ def _classify_list(rows, source): if r['name'] and not r['malformed_file_id'] and (r['file_id'] is None or source == 'remote')] by_name = lookup_by_names(names, source) + # a bare number (no name) may be a page ID or revision ID someone found + # on Commons; nothing confirms it is a file_id + bare = [r['file_id'] for r in rows if r['file_id'] is not None and not r['name']] + others = lookup_other_ids(bare) if bare and source != 'remote' else {} ret = [] for r in rows: - if r['malformed_file_id']: + other = others.get(r['file_id']) if not r['name'] else None + rec = by_id.get(r['file_id']) if other else None + if other and (rec is None or rec['img_name'] != other['name']): + ret.append(_other_id_row(r, other, rec)) + elif r['malformed_file_id']: ret.append(_row(r, 'malformed_file_id', reason='file_id "%s" is not a plain whole number' % r['file_id_as_written'].strip())) @@ -226,6 +242,27 @@ def _classify_list(rows, source): return ret +_ID_KINDS = {'page_id': 'the page ID', 'revision_id': 'a revision ID'} + + +def _other_id_row(r, other, rec): + """A bare number that is a File: page's page ID or revision ID.""" + num, page = r['file_id'], 'File:' + other['name'] + if rec is not None: + return _row(r, 'ambiguous_id', file_id=num, + reason='%s is the file_id of %s but also %s of %s; write' + ' the file name instead, so it is clear which file is meant' + % (num, rec['img_name'], _ID_KINDS[other['kind']], page)) + if other['kind'] == 'revision_id': + reason = ('%s is a revision ID (one edit of the page %s), not a file_id;' + ' Montage imports files, so write the file name instead: %s' + % (num, page, other['name'])) + else: + reason = ('%s is the page ID of %s, not a file_id; write the file name' + ' instead: %s' % (num, page, other['name'])) + return _row(r, other['kind'], file_id=num, reason=reason) + + def _file_key(row): if row['file_id'] is not None: return ('file_id', row['file_id']) diff --git a/montage/labs.py b/montage/labs.py index c7223b91..2a51304e 100644 --- a/montage/labs.py +++ b/montage/labs.py @@ -215,6 +215,32 @@ def get_files_info_by_ids(file_ids): return ret +def get_file_pages_by_other_ids(numbers): + """{number: {'kind': 'page_id' | 'revision_id', 'name': file name}} for + the numbers that are the page ID, or a revision ID, of a File: page. + Those are the IDs people find on Commons and may take for a file_id + (hatnote/montage#510). A page ID wins over a revision ID.""" + nums = sorted(set(int(n) for n in numbers)) + ret = {} + if not nums: + return ret + with commonswiki_connection(COMMONS_DB_HOST) as fetchall: + for i in range(0, len(nums), FILE_LOOKUP_CHUNK_SIZE): + chunk = tuple(nums[i:i + FILE_LOOKUP_CHUNK_SIZE]) + marks = ', '.join(['%s'] * len(chunk)) + revs = fetchall( + 'SELECT rev_id AS num, page_title AS name FROM revision' + ' JOIN page ON rev_page = page_id' + ' WHERE page_namespace = 6 AND rev_id IN (%s)' % marks, chunk) + pages = fetchall( + 'SELECT page_id AS num, page_title AS name FROM page' + ' WHERE page_namespace = 6 AND page_id IN (%s)' % marks, chunk) + for kind, rows in (('revision_id', revs), ('page_id', pages)): + for rec in rows: + ret[int(rec['num'])] = {'kind': kind, 'name': rec['name']} + return ret + + 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. diff --git a/montage/loaders.py b/montage/loaders.py index afc65189..bc6438f8 100644 --- a/montage/loaders.py +++ b/montage/loaders.py @@ -14,7 +14,8 @@ from boltons.iterutils import chunked_iter import montage.rdb # TODO: circular import -from .labs import get_files, get_files_info_by_names, get_files_info_by_ids +from .labs import (get_files, get_files_info_by_names, get_files_info_by_ids, + get_file_pages_by_other_ids) from .utils import (ImportSourceInvalid, requests_get, requests_post) @@ -412,6 +413,12 @@ def lookup_by_ids(file_ids): return get_files_info_by_ids(file_ids) +def lookup_other_ids(numbers): + """{number: {kind, name}} for numbers that are a File: page's page ID or + revision ID rather than a file_id; on the wikireplica only.""" + return get_file_pages_by_other_ids(numbers) + + def category_records(category_name, source='local'): """Commons file infos of a category's files.""" if category_name is not None and not isinstance(category_name, str): diff --git a/montage/tests/test_import_check.py b/montage/tests/test_import_check.py index 7dc23a4f..93f2d7ce 100644 --- a/montage/tests/test_import_check.py +++ b/montage/tests/test_import_check.py @@ -39,9 +39,10 @@ def info(name, file_id, **kw): class FakeCommons(object): """Stands in for the wikireplica lookups (local mode).""" - def __init__(self, files): + def __init__(self, files, other_ids=None): self.by_name = {f['img_name']: f for f in files} self.by_id = {f['file_id']: f for f in files} + self.other_ids = other_ids or {} self.calls = [] def names(self, names, source='local'): @@ -52,6 +53,10 @@ def ids(self, ids): self.calls.append(('ids', sorted(set(ids)))) return {i: self.by_id[i] for i in ids if i in self.by_id} + def others(self, numbers): + self.calls.append(('other_ids', sorted(set(numbers)))) + return {n: self.other_ids[n] for n in numbers if n in self.other_ids} + def category(self, name, source='local'): self.calls.append(('category', name)) return sorted(self.by_name.values(), key=lambda f: f['img_name'].encode('utf8')) @@ -61,12 +66,13 @@ def category(self, name, source='local'): def commons(monkeypatch): fake = FakeCommons([]) - def install(files): - fake.__init__(files) + def install(files, other_ids=None): + fake.__init__(files, other_ids) return fake monkeypatch.setattr(import_check, 'lookup_by_names', fake.names) monkeypatch.setattr(import_check, 'lookup_by_ids', fake.ids) monkeypatch.setattr(import_check, 'category_records', fake.category) + monkeypatch.setattr(import_check, 'lookup_other_ids', fake.others) return install @@ -400,6 +406,38 @@ def test_file_list_of_bare_file_ids(commons): (4, 'unknown_file_id')] assert result['rows'][0]['commons_name'] == VOLOCHEK + +def test_bare_page_and_revision_ids_are_refused_with_a_reason(commons): + """People find page IDs and revision IDs on Commons, not file_ids. A + bare number that is one of those blocks, saying what it is and which + file it points to, instead of 'not on Commons' or, worse, importing + another file whose file_id happens to be that number.""" + commons([info('A.jpg', 1), info('B.jpg', 2)], + other_ids={1: {'kind': 'page_id', 'name': 'A.jpg'}, # same file: fine + 2: {'kind': 'page_id', 'name': 'Other.jpg'}, # ambiguous + 1100000000: {'kind': 'revision_id', 'name': 'C.jpg'}, + 170000000: {'kind': 'page_id', 'name': 'D.jpg'}}) + result = run_check({'import_method': 'selected', + 'file_names': ['1', '2', '1100000000', '170000000', '5', 'B.jpg']}, + CAMPAIGN, source='local') + assert statuses(result) == [(1, 'ok'), (2, 'ambiguous_id'), (3, 'revision_id'), + (4, 'page_id'), (5, 'unknown_file_id'), (6, 'ok')] + reasons = {r['row']: r['reason'] for r in result['rows']} + assert 'revision ID' in reasons[3] and 'C.jpg' in reasons[3] + assert 'page ID' in reasons[4] and 'D.jpg' in reasons[4] + assert 'B.jpg' in reasons[2] and 'Other.jpg' in reasons[2] + assert [r['row'] for r in blocking_rows(result)] == [2, 3, 4, 5] + + +def test_named_rows_are_not_looked_up_as_page_ids(commons): + """A row with a name confirms its file_id; only bare numbers are + checked against page and revision IDs.""" + fake = commons([info('A.jpg', 1)], other_ids={1: {'kind': 'page_id', 'name': 'X.jpg'}}) + result = run_check({'import_method': 'selected', 'file_names': ['A.jpg']}, + CAMPAIGN, source='local') + assert statuses(result) == [(1, 'ok')] + assert not any(c[0] == 'other_ids' for c in fake.calls) + # --------------------------------------------------------------------------- # Check files # --------------------------------------------------------------------------- diff --git a/montage/tests/test_loaders.py b/montage/tests/test_loaders.py index 29cadbae..0c5acd51 100644 --- a/montage/tests/test_loaders.py +++ b/montage/tests/test_loaders.py @@ -582,6 +582,20 @@ def test_get_files_info_by_ids_live(): assert by_id[by_name['file_id']] == by_name +@pytest.mark.xfail( + os.environ.get('TOOLFORGE') != '1', + reason='Requires live wikireplicas (Toolforge); set TOOLFORGE=1 to run', +) +def test_get_file_pages_by_other_ids_live(): + """The page ID and a revision ID of a File: page (as shown on Commons, + 2026-10-07) are recognised as such.""" + from montage.labs import get_file_pages_by_other_ids + name = 'Reynisfjara,_Suðurland,_Islandia,_2014-08-17,_DD_164.JPG' + found = get_file_pages_by_other_ids([37348796, 1281700543]) + assert found[37348796] == {'kind': 'page_id', 'name': name} + assert found[1281700543] == {'kind': 'revision_id', 'name': name} + + def test_source_url_rewrites_only_gist_links(): """#208: a plain CSV / text link got '/raw' appended and returned 404.""" from montage.loaders import source_url From aaf5d3bb8a4902f365ca6e1f79b611fea940461b Mon Sep 17 00:00:00 2001 From: lgelauff <32471480+lgelauff@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:33:47 +0000 Subject: [PATCH 17/20] fix(#510): import check UI review: translated reasons, clearer texts, focus, phones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a three-viewpoint UI review (coordinator, consistency, screen reader + phone), 2026-10-08. Translated reasons: every checked row and every source error now carries reason_code + reason_params; the form shows montage-round-check-reason-* / montage-round-check-error-* and falls back to the English server text (marked lang="en") only without a translation. The dialog after saving is built from the check's rows, translated, instead of server English. Texts (one glossary: problems, file ID, "check the source again", "can be imported", Commons, group): - a check with only warnings now says the round can be saved and that rows are left out or changed; - the hints next to Save no longer point to rows "below" (they are above); Save after a blocking check no longer says "check first"; - same-name: "names Montage cannot tell apart", explains why (case, accents, е/ё), "groups" instead of "pairs", no advice to rename; - labels match the help text (file names or file IDs; link to the sheet or file list); expiry says nothing was saved and why; - the partial-download link is short, the steps follow as text; - blocked-save detail: no raw status codes, singular "1 row". Screen reader and phone: - focus moves to the check result or the save error after a check, an expired check or a refused save (Save loses focus while disabled); the result panel is a named region instead of being announced twice; the live region is outside the aria-busy container; - Save is disabled while a check runs; - buttons and the button row wrap instead of cutting labels off; the problems table scrolls sideways on phones (no inner vertical scroll); row numbers are row headers; file names in for right-to-left names; dialog text wraps long names. Tests: reason codes per row and on source errors; wording updates. hatnote/montage#510 Co-Authored-By: Claude Opus 5.5 --- .../components/Round/ImportCheckResult.vue | 95 ++++++++-- frontend/src/components/Round/RoundNew.vue | 175 ++++++++++++------ .../src/components/Round/importCheckText.js | 35 ++++ frontend/src/i18n/en.json | 73 ++++++-- frontend/src/i18n/qqq.json | 53 +++++- frontend/src/services/dialogService.js | 6 +- montage/docs/api.md | 6 +- montage/import_check.py | 90 +++++---- montage/loaders.py | 62 ++++--- montage/tests/test_import_check.py | 41 +++- montage/tests/test_loaders.py | 3 +- montage/utils.py | 10 + 12 files changed, 476 insertions(+), 173 deletions(-) create mode 100644 frontend/src/components/Round/importCheckText.js diff --git a/frontend/src/components/Round/ImportCheckResult.vue b/frontend/src/components/Round/ImportCheckResult.vue index f7f42434..607b46dd 100644 --- a/frontend/src/components/Round/ImportCheckResult.vue +++ b/frontend/src/components/Round/ImportCheckResult.vue @@ -1,11 +1,29 @@