The_Great_Archive/ArchiveBox
1
0
Fork 0
mirror of https://github.com/ArchiveBox/ArchiveBox.git synced 2026-09-12 19:50:57 +05:00
Code Issues Actions 4 Packages Projects Releases Wiki Activity

Address Cubic review feedback

Browse Source
This commit is contained in:
Nick Sweeting 2026-08-28 21:54:07 -07:00
parent c062a3c54f
commit 97486882bd
No known key found for this signature in database
11 changed files with 163 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
archivebox/api/v1_core.py
View File

@ -944,6 +944,9 @@ def create_snapshot(request: HttpRequest, data: SnapshotCreateSchema):
created_by=request.user if isinstance(request.user, User) else None,
)
# Browser uploads must not wait behind the runner's crawl-wide lifecycle
# lock. The unique insert recovery and CAS update below are the request-side
# coordination boundary for this idempotent metadata sync.
snapshot = Snapshot.objects.filter(url=data.url, crawl=crawl).first()
if snapshot is None:
try:
@ -973,7 +976,10 @@ def create_snapshot(request: HttpRequest, data: SnapshotCreateSchema):
raise HttpError(409, "Snapshot changed while metadata was being updated")
if tags:
snapshot.save_tags(tags)
snapshot.save_tags(
tags,
created_by=request.user if isinstance(request.user, User) else None,
)
try:
snapshot.ensure_crawl_symlink()
@ -1026,7 +1032,10 @@ def patch_snapshot(request: HttpRequest, snapshot_id: str, data: SnapshotUpdateS
update_fields.append("retry_at")
if tags is not None:
snapshot.save_tags(normalize_tag_list(tags))
snapshot.save_tags(
normalize_tag_list(tags),
created_by=request.user if isinstance(request.user, User) else None,
)
if payload.get("status") == Snapshot.StatusChoices.SEALED:
snapshot.cancel()

10
archivebox/core/admin_snapshots.py
View File

@ -356,6 +356,16 @@ class SnapshotAdmin(SearchResultsAdminMixin, ConfigEditorMixin, BaseModelAdmin):
form = SnapshotAdminForm
raw_id_fields = ("crawl", "parent_snapshot")
list_select_related = ()
def save_related(self, request, form, formsets, change):
super().save_related(request, form, formsets, change)
tags_str = form.cleaned_data.get("tags_editor", "")
tag_names = [name.strip() for name in tags_str.split(",") if name.strip()]
form.instance.save_tags(
tag_names,
created_by=request.user if request.user.is_authenticated else None,
)
list_display = (
"permissions_badge",
"created_at",

12
archivebox/core/models.py
View File

@ -609,9 +609,11 @@ class Snapshot(ModelWithDeleteAfter, ModelWithOutputDir, ModelWithConfig, ModelW
try:
SnapshotTag(snapshot_id=self.pk, tag_id=tag_id).save(force_insert=True)
except IntegrityError:
# The unique (snapshot, tag) row already exists. In autocommit
# mode the failed INSERT is fully rolled back before continuing.
continue
# Only the unique (snapshot, tag) conflict is idempotent. Do
# not hide foreign-key or other integrity failures.
if SnapshotTag.objects.filter(snapshot_id=self.pk, tag_id=tag_id).exists():
continue
raise
def remove_tag_ids(self, tag_ids: Iterable[int | str]) -> int:
tag_ids = [tag_id for tag_id in dict.fromkeys(tag_ids) if tag_id]
@ -2726,10 +2728,10 @@ class Snapshot(ModelWithDeleteAfter, ModelWithOutputDir, ModelWithConfig, ModelW
def archive_size(self):
return int(self.output_size or 0)
def save_tags(self, tags: Iterable[str] = ()) -> None:
def save_tags(self, tags: Iterable[str] = (), *, created_by: Any = None) -> None:
from archivebox.core.tag_util import get_or_create_tag
tag_ids = {get_or_create_tag(tag)[0].pk for tag in tags if tag.strip()}
tag_ids = {get_or_create_tag(tag, created_by=created_by)[0].pk for tag in tags if tag.strip()}
existing_tag_ids = set(SnapshotTag.objects.filter(snapshot_id=self.pk).values_list("tag_id", flat=True))
self.remove_tag_ids(existing_tag_ids - tag_ids)
self.add_tag_ids(tag_ids - existing_tag_ids)

3
archivebox/core/tag_util.py
View File

@ -152,7 +152,8 @@ def get_or_create_tag(name: str, created_by: User | None = None) -> tuple[Tag, b
if not normalized_name:
raise ValueError("Tag name is required")
return Tag.get_or_create_by_name(normalized_name, defaults={"created_by": created_by})
defaults = {"created_by": created_by} if created_by is not None else None
return Tag.get_or_create_by_name(normalized_name, defaults=defaults)
def rename_tag(tag: Tag, name: str) -> Tag:

21
archivebox/services/archive_result_service.py
View File

@ -332,20 +332,13 @@ def _save_archiveresult_event_to_db(
if event.error:
defaults["notes"] = event.error
with _perf_span("archivebox.ArchiveResultService.on_ArchiveResultEvent.result_lookup"):
result = ArchiveResult.objects.filter(
snapshot=snapshot,
plugin=event.plugin,
hook_name=event.hook_name,
).first()
if result is None:
with _perf_span("archivebox.ArchiveResultService.on_ArchiveResultEvent.result_create"):
result, _created = ArchiveResult.get_or_create_by_hook(
snapshot,
event.plugin,
event.hook_name,
defaults=defaults,
)
with _perf_span("archivebox.ArchiveResultService.on_ArchiveResultEvent.result_get_or_create"):
result, _created = ArchiveResult.get_or_create_by_hook(
snapshot,
event.plugin,
event.hook_name,
defaults=defaults,
)
with _perf_span("archivebox.ArchiveResultService.on_ArchiveResultEvent.diff_fields"):
update_fields = []

5
archivebox/tests/test_api_v1_core_archiveresults.py
View File

@ -69,8 +69,9 @@ def test_archiveresult_create_does_not_open_a_database_transaction(client, api_a
)
assert response.status_code == 200, response.content
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []
if connection.vendor == "sqlite":
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []
def test_intermediate_archiveresult_chunks_only_write_to_disk(client, api_admin_user, api_headers):

42
archivebox/tests/test_api_v1_core_snapshots.py
View File

@ -110,5 +110,43 @@ def test_new_snapshot_creation_does_not_open_a_database_transaction(client, api_
assert response.status_code == 200, response.content
assert Snapshot.objects.filter(url=url, crawl=crawl).count() == 1
assert Snapshot.objects.get(url=url, crawl=crawl).tags.filter(name="browser-extension-upload").exists()
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []
if connection.vendor == "sqlite":
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []
def test_new_snapshot_creation_does_not_wait_for_active_crawl(client, api_admin_user, api_headers):
url = "https://example.com/browser-extension-new-snapshot-active-crawl"
crawl = Crawl.objects.create(urls=url, created_by=api_admin_user)
lock_acquired = threading.Event()
release_lock = threading.Event()
def hold_active_crawl_lock():
with crawl_lifecycle_lock(str(crawl.id)):
lock_acquired.set()
release_lock.wait(timeout=3)
holder = threading.Thread(target=hold_active_crawl_lock)
holder.start()
assert lock_acquired.wait(timeout=1)
started_at = time.monotonic()
response = client.post(
"/api/v1/core/snapshots",
data={
"url": url,
"crawl_id": str(crawl.id),
"depth": 0,
"status": Snapshot.StatusChoices.QUEUED,
"tags": ["browser-extension-upload"],
},
content_type="application/json",
**api_headers,
)
elapsed = time.monotonic() - started_at
release_lock.set()
holder.join(timeout=3)
assert response.status_code == 200, response.content
assert Snapshot.objects.filter(url=url, crawl=crawl).count() == 1
assert elapsed < 1

28
archivebox/tests/test_api_v1_core_tags_add_to_snapshot.py
View File

@ -1,5 +1,5 @@
import pytest
from django.db import connection
from django.db import IntegrityError, connection
from django.test.utils import CaptureQueriesContext
from archivebox.core.models import Snapshot, Tag
@ -27,5 +27,27 @@ def test_basic_success_case_request(client, api_admin_user, api_headers):
assert response.status_code == 200, response.content
assert response.json()["success"] is True
assert snapshot.tags.filter(pk=tag.pk).exists()
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []
if connection.vendor == "sqlite":
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []
def test_add_tag_ids_reraises_non_duplicate_integrity_errors(snapshot):
with pytest.raises(IntegrityError):
snapshot.add_tag_ids([2**31 - 1])
def test_add_tag_ids_treats_existing_snapshot_tag_as_idempotent(snapshot, admin_user):
tag = Tag.objects.create(name="already-attached", created_by=admin_user)
snapshot.add_tag_ids([tag.pk])
snapshot.add_tag_ids([tag.pk])
assert snapshot.tags.filter(pk=tag.pk).count() == 1
def test_save_tags_without_creator_uses_tag_model_default(snapshot):
snapshot.save_tags(["model-default-creator"])
tag = Tag.objects.get(name="model-default-creator")
assert tag.created_by_id is not None

5
archivebox/tests/test_api_v1_core_tags_remove_from_snapshot.py
View File

@ -28,5 +28,6 @@ def test_basic_success_case_request(client, api_admin_user, api_headers):
assert response.status_code == 200, response.content
assert response.json()["success"] is True
assert not snapshot.tags.filter(pk=tag.pk).exists()
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []
if connection.vendor == "sqlite":
transaction_queries = [query["sql"] for query in queries if query["sql"].strip().upper() in {"BEGIN", "COMMIT"}]
assert transaction_queries == []

26
archivebox/tests/test_archive_result_service.py
View File

@ -251,6 +251,32 @@ def test_archiveresult_duplicate_hook_rows_are_rejected():
)
def test_archiveresult_event_create_uses_one_result_lookup():
from abx_dl.events import ArchiveResultEvent
from django.db import connection
from django.test.utils import CaptureQueriesContext
from archivebox.services.archive_result_service import _save_archiveresult_event_to_db
snapshot = _create_snapshot()
event = ArchiveResultEvent(
snapshot_id=str(snapshot.id),
plugin="review-query-count",
hook_name="on_Snapshot__99_review.py",
status="failed",
)
with CaptureQueriesContext(connection) as queries:
_save_archiveresult_event_to_db(event, None)
result_lookups = [
query["sql"]
for query in queries
if query["sql"].lstrip().upper().startswith("SELECT") and 'FROM "core_archiveresult"' in query["sql"]
]
assert len(result_lookups) == 1
def test_process_completed_projects_failed_archiveresult_from_shipped_hook(tmp_path, hermetic_lib_dir):
from archivebox.core.models import ArchiveResult

29
archivebox/tests/test_ui_admin_snapshot.py
View File

@ -311,6 +311,35 @@ def test_snapshot_admin_preview_uses_extension_screenshot_when_standard_screensh
assert "chrome_extension_screenshot/screenshot-2.png" not in preview["fallback_list"]
def test_snapshot_admin_attributes_new_tags_to_authenticated_user(client, snapshot, admin_user):
from archivebox.core.models import Tag
client.force_login(admin_user)
response = client.post(
reverse("admin:core_snapshot_change", args=[snapshot.pk]),
{
"url": snapshot.url,
"title": snapshot.title or "",
"tags_editor": "admin-created-tag",
"permissions_config": "private",
"status": snapshot.status,
"retry_at": "",
"bookmarked_at_0": snapshot.bookmarked_at.date().isoformat(),
"bookmarked_at_1": snapshot.bookmarked_at.time().isoformat(),
"crawl": str(snapshot.crawl_id),
"config": '{"SAVE_ARCHIVE_DOT_ORG": "false"}',
"notes": "",
"_save": "Save",
},
HTTP_HOST=ADMIN_TEST_HOST,
)
assert response.status_code == 302, response.context and response.context["adminform"].form.errors
tag = Tag.objects.get(name="admin-created-tag")
assert tag.created_by == admin_user
assert snapshot.tags.filter(pk=tag.pk).exists()
class TestSnapshotProgressStats:
"""Tests for Snapshot.get_progress_stats() method."""