diff --git a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py index e08651c8f9..2942d2c0f3 100644 --- a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py +++ b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py @@ -14,6 +14,7 @@ from celery import shared_task from openpyxl import Workbook from openpyxl.cell import WriteOnlyCell +from openpyxl.cell.cell import ILLEGAL_CHARACTERS_RE from openpyxl.styles.numbers import FORMAT_TEXT from sqlalchemy import ( bindparam, @@ -268,21 +269,40 @@ def _stringify_cell_value(value: Any) -> str: return str(value) +def _sanitize_cell_value(value: str) -> str: + """Strip characters that Excel does not permit in worksheet cells. + + Some imported metadata contains stray control characters (e.g. a U+001F unit + separator embedded in a contributor name). openpyxl rejects these with an + ``IllegalCharacterError``, which would fail the whole report, so we remove + them. The same regex openpyxl validates with is used here, so anything that + survives is guaranteed to be writable. Tab, newline and carriage return are + not matched by that regex and are therefore preserved. + """ + return ILLEGAL_CHARACTERS_RE.sub("", value) + + def _cell_value(key: str, value: Any, stringify_cols: frozenset[str]) -> Any: """Convert a cell value for report output (shared by CSV and Excel writers). For columns in stringify_cols, forces the value to a plain string. Enum values are converted using their .value attribute. Timezone-aware datetimes are formatted as strings for Excel compatibility. + String values from the database are stripped of characters that Excel + disallows, so that the CSV and Excel outputs stay identical. """ if key in stringify_cols: - return _stringify_cell_value(value) + return _sanitize_cell_value(_stringify_cell_value(value)) if value is None: return "" if isinstance(value, enum.Enum): + # Enum values are declared in our own source, so they can't carry + # illegal characters and don't need sanitizing. return str(value.value) if isinstance(value, datetime) and value.tzinfo is not None: return value.strftime("%Y-%m-%d %H:%M:%S.%f") + if isinstance(value, str): + return _sanitize_cell_value(value) return value diff --git a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py index 58bec1ddc9..f3dc0a4d23 100644 --- a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py +++ b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py @@ -277,6 +277,79 @@ def test_generate_csv_report_quotes_target_age( assert '"5-8"' in csv_content +def test_generate_excel_report_strips_illegal_characters( + db: DatabaseTransactionFixture, +): + """Control characters in the data don't blow up Excel generation. + + Imported metadata sometimes carries stray control characters (e.g. a U+001F + unit separator inside a contributor name). openpyxl refuses to write them, + so they are stripped rather than failing the whole report. + """ + query = select( + literal("Ja\x1fne Doe").label("author"), + literal("Ti\x0btle").label("title"), + literal("line1\nline2\ttabbed").label("notes"), + literal("97803064\x1f06157").label("identifier"), + ) + + excel_file = io.BytesIO() + excel_file.name = "test_report.xlsx" + + generate_excel_report( + db=db.session, + excel_file=excel_file, + sql_params={}, + query=query, + columns_to_stringify={"identifier"}, + ) + + excel_file.seek(0) + wb = load_workbook(io.BytesIO(excel_file.getvalue())) + ws = wb.active + assert ws is not None + headers = [ws.cell(row=1, column=c).value for c in range(1, ws.max_column + 1)] + values = { + header: ws.cell(row=2, column=index + 1).value + for index, header in enumerate(headers) + } + assert values["author"] == "Jane Doe" + # Stringified columns go through a separate branch of the sanitizer. + assert values["identifier"] == "9780306406157" + assert values["title"] == "Title" + # Tab, newline and carriage return are legal and must be preserved. + assert values["notes"] == "line1\nline2\ttabbed" + + +def test_generate_csv_report_strips_illegal_characters( + db: DatabaseTransactionFixture, +): + """The CSV output is sanitized identically to the Excel output. + + ``identifier`` is a stringified column, so it exercises the + ``_stringify_cell_value`` branch of the sanitizer as well. + """ + query = select( + literal("Ja\x1fne Doe").label("author"), + literal("97803064\x1f06157").label("identifier"), + ) + + csv_file = io.StringIO() + csv_file.name = "test_report.csv" + + generate_csv_report( + db=db.session, + csv_file=csv_file, + sql_params={}, + query=query, + columns_to_stringify={"identifier"}, + ) + + csv_file.seek(0) + rows = list(csv.reader(io.StringIO(csv_file.getvalue()))) + assert rows[1] == ["Jane Doe", "9780306406157"] + + def test_only_active_collections_are_included( db: DatabaseTransactionFixture, services_fixture: ServicesFixture ):