diff --git a/modules/archive/api.py b/modules/archive/api.py index cd912fa7..5dca4bdf 100644 --- a/modules/archive/api.py +++ b/modules/archive/api.py @@ -38,9 +38,13 @@ async def statistics(request: Request): async def get_archive(request: Request, file_group_id: int): session = request.ctx.session skip_viewed = request.args.get('skip_viewed', '').lower() == 'true' - archive = lib.get_archive_by_file_group_id(session, file_group_id, skip_viewed=skip_viewed) + archive = lib.get_archive_by_file_group_id(session, file_group_id, skip_viewed=True) archive_file_group = archive.file_group.__json__() history = [i.file_group.__json__() for i in archive.history] + if not skip_viewed: + # Marking the Archive viewed is a write; it must not upgrade the read above (see `RequestDB.write`). + with request.ctx.db.write(): + archive_file_group['viewed'] = archive.file_group.set_viewed() return json_response({'file_group': archive_file_group, 'history': history}) diff --git a/modules/archive/test/test_api.py b/modules/archive/test/test_api.py index fb69d3a4..ee54588a 100644 --- a/modules/archive/test/test_api.py +++ b/modules/archive/test/test_api.py @@ -7,6 +7,10 @@ import pytest from modules.archive import lib, Archive from modules.archive.lib import UploadedArchive from wrolpi.common import get_relative_to_media_directory +from wrolpi.conftest import production_like_sessions +from wrolpi.db import get_db_session +from wrolpi.files.models import FileGroup +from wrolpi.tags import Tag from wrolpi.test.common import skip_ci, skip_macos @@ -632,3 +636,33 @@ async def test_get_archive_loads_history_eagerly(test_session, archive_factory, assert all(i['tags'] == [tag.name] for i in long_json['history']) # Five history Archives cost no more queries than one. assert long_statements == short_statements, (short_statements, long_statements) + + +@pytest.mark.asyncio +async def test_get_archive_records_view_after_a_concurrent_write(test_session, archive_factory, async_client): + """Viewing an Archive still succeeds, and is recorded, when another connection commits during the request.""" + archive = archive_factory('example.com', 'https://example.com/page') + archive_factory('example.com', 'https://example.com/page') + file_group_id = archive.file_group_id + test_session.commit() + + __json__ = FileGroup.__json__ + concurrent_writes = [] + + def json_after_a_concurrent_write(self): + # A download or refresh commits while the page is being read. + if not concurrent_writes: + with get_db_session(commit=True) as session: + session.add(Tag(name='concurrent', color='#000000')) + concurrent_writes.append(True) + return __json__(self) + + with production_like_sessions(test_session), \ + patch.object(FileGroup, '__json__', json_after_a_concurrent_write): + request, response = await async_client.get(f'/api/archive/{file_group_id}') + assert response.status_code == HTTPStatus.OK, response.json + assert len(response.json['history']) == 1 + assert response.json['file_group']['viewed'] + + test_session.rollback() # End the test's own read snapshot to see the request's commit. + assert test_session.query(FileGroup).filter_by(id=file_group_id).one().viewed is not None diff --git a/modules/docs/api.py b/modules/docs/api.py index 41de7356..13fb0bca 100644 --- a/modules/docs/api.py +++ b/modules/docs/api.py @@ -31,8 +31,13 @@ async def statistics(request: Request): async def get_doc(request: Request, file_group_id: int): session = request.ctx.session skip_viewed = request.args.get('skip_viewed', '').lower() == 'true' - doc = _get_doc(session, file_group_id, skip_viewed=skip_viewed) - return json_response(_doc_response(doc)) + doc = _get_doc(session, file_group_id, skip_viewed=True) + response = _doc_response(doc) + if not skip_viewed: + # Marking the doc viewed is a write; it must not upgrade the read above (see `RequestDB.write`). + with request.ctx.db.write(): + response['file_group']['viewed'] = doc.file_group.set_viewed() + return json_response(response) doc_limit_limiter = api_param_limiter(100) diff --git a/modules/docs/test/test_api.py b/modules/docs/test/test_api.py index d0ed5507..eeb69de3 100644 --- a/modules/docs/test/test_api.py +++ b/modules/docs/test/test_api.py @@ -1,10 +1,15 @@ """Tests for the docs API.""" import json from http import HTTPStatus +from unittest.mock import patch import pytest from modules.docs.models import Doc +from wrolpi.conftest import production_like_sessions +from wrolpi.db import get_db_session +from wrolpi.files.models import FileGroup +from wrolpi.tags import Tag @pytest.mark.asyncio @@ -169,3 +174,30 @@ async def test_delete_doc_atomic_when_some_tagged(async_client, test_session, do assert all(p.is_file() for p in untagged_paths) assert all(p.is_file() for p in tagged_paths) assert test_session.query(Doc).count() == 2 + + +@pytest.mark.asyncio +async def test_get_doc_records_view_after_a_concurrent_write(async_client, test_session, doc_factory): + """Viewing a doc still succeeds, and is recorded, when another connection commits during the request.""" + file_group_id = doc_factory().file_group_id + test_session.commit() + + __json__ = FileGroup.__json__ + concurrent_writes = [] + + def json_after_a_concurrent_write(self): + # A download or refresh commits while the doc is being read. + if not concurrent_writes: + with get_db_session(commit=True) as session: + session.add(Tag(name='concurrent', color='#000000')) + concurrent_writes.append(True) + return __json__(self) + + with production_like_sessions(test_session), \ + patch.object(FileGroup, '__json__', json_after_a_concurrent_write): + request, response = await async_client.get(f'/api/docs/{file_group_id}') + assert response.status_code == HTTPStatus.OK, response.json + assert response.json['file_group']['viewed'] + + test_session.rollback() # End the test's own read snapshot to see the request's commit. + assert test_session.query(FileGroup).filter_by(id=file_group_id).one().viewed is not None