Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions invenio_records_rest/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,20 @@ def __init__(self, **kwargs):
super().__init__(**kwargs)


class RecordConflictRESTError(RESTException):
"""Record was modified concurrently."""

code = 409

def __init__(self, **kwargs):
"""Initialize exception."""
if "description" not in kwargs:
kwargs["description"] = _(
"The record was modified concurrently, please retry."
)
super().__init__(**kwargs)


class SuggestMissingContextRESTError(RESTException):
"""Missing a context value when getting record suggestions."""

Expand Down
17 changes: 14 additions & 3 deletions invenio_records_rest/views.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# SPDX-FileCopyrightText: 2015-2019 CERN.
# SPDX-FileCopyrightText: 2023-2026 Graz University of Technology.
# SPDX-FileCopyrightText: 2026 RERO.
# SPDX-License-Identifier: MIT

"""REST API resources."""
Expand Down Expand Up @@ -32,6 +33,7 @@
from jsonpatch import JsonPatchException, JsonPointerException
from jsonschema.exceptions import ValidationError
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.orm.exc import StaleDataError
from webargs import ValidationError as WebargsValidationError
from webargs import fields, validate
from webargs.flaskparser import parser
Expand All @@ -44,6 +46,7 @@
JSONSchemaValidationError,
PatchJSONFailureRESTError,
PIDResolveRESTError,
RecordConflictRESTError,
SearchPaginationRESTError,
SuggestMissingContextRESTError,
SuggestNoCompletionsRESTError,
Expand Down Expand Up @@ -377,10 +380,14 @@ def pass_record(f):
@wraps(f)
def inner(self, pid_value, *args, **kwargs):
try:
# .data is a cached_property that triggers the actual DB queries
# (PersistentIdentifier lookup + Record.get_record) via the
# Werkzeug PIDConverter lazy resolver. SQLAlchemyError can be
# raised here if the DB is unavailable or the connection is lost.
pid, record = request.view_args["pid_value"].data
return f(self, pid=pid, record=record, *args, **kwargs)
except SQLAlchemyError:
raise PIDResolveRESTError(pid_value)
return f(self, pid=pid, record=record, *args, **kwargs)

return inner

Expand Down Expand Up @@ -945,8 +952,12 @@ def put(self, pid, record, **kwargs):

record.clear()
record.update(data)
record.commit()
db.session.commit()
try:
record.commit()
db.session.commit()
except StaleDataError:
db.session.rollback()
raise RecordConflictRESTError()
if self.indexer_class:
self.indexer_class().index(record)
return self.make_response(pid, record, links_factory=self.links_factory)
Expand Down
4 changes: 3 additions & 1 deletion tests/test_views_item_delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
from mock import patch
from sqlalchemy.exc import SQLAlchemyError

from invenio_records_rest.errors import PIDResolveRESTError


def test_valid_delete(app, indexed_records):
"""Test VALID record delete request (DELETE .../records/<record_id>)."""
Expand Down Expand Up @@ -53,7 +55,7 @@ def test_delete_with_sqldatabase_error(app, indexed_records):
with app.test_client() as client:

def raise_error():
raise SQLAlchemyError()
raise PIDResolveRESTError()

# Force an SQLAlchemy error that will rollback the transaction.
with patch.object(PersistentIdentifier, "delete", side_effect=raise_error):
Expand Down
50 changes: 50 additions & 0 deletions tests/test_views_item_put.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# SPDX-FileCopyrightText: 2015-2018 CERN.
# SPDX-FileCopyrightText: 2026 RERO.
# SPDX-License-Identifier: MIT

"""Record PUT tests."""
Expand All @@ -9,6 +10,8 @@
import pytest
from conftest import IndexFlusher
from helpers import _mock_validate_fail, assert_hits_len, get_json, record_url
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.orm.exc import StaleDataError


@pytest.mark.parametrize(
Expand Down Expand Up @@ -162,3 +165,50 @@ def test_validation_error(app, test_records, content_type):
url = record_url(pid)
res = client.put(url, data=json.dumps(record.dumps()), headers=HEADERS)
assert res.status_code == 400


def test_put_stale_data_returns_409(app, db, test_records):
"""PUT on a stale record returns 409, not 500 PIDResolveRESTError.

SQLAlchemy optimistic-locking raises StaleDataError when two concurrent
requests read revision N and the second one tries to commit after the first
already bumped the revision to N+1. The fix moves the return inside put()
outside pass_record's try/except so the error is handled locally as a 409.
"""
HEADERS = [("Accept", "application/json"), ("Content-Type", "application/json")]
pid, record = test_records[0]

with mock.patch(
"invenio_db.db.session.commit", side_effect=StaleDataError("stale")
):
with app.test_client() as client:
url = record_url(pid)
res = client.put(url, data=json.dumps(record.dumps()), headers=HEADERS)
assert res.status_code == 409
data = get_json(res)
assert data["status"] == 409
assert "concurrently" in data["message"]


def test_pass_record_does_not_swallow_handler_sqlalchemy_errors(app, db, test_records):
"""SQLAlchemyError from the handler must not be caught by pass_record.

Before the fix, pass_record wrapped the handler call inside the
try/except SQLAlchemyError used for PID resolution, so any database error
raised inside put() (e.g. IntegrityError) would be silently re-raised as a
PIDResolveRESTError(500) with a misleading "PID could not be resolved"
message. After the fix, such errors propagate unmodified.
"""
HEADERS = [("Accept", "application/json"), ("Content-Type", "application/json")]
pid, record = test_records[0]

with mock.patch(
"invenio_db.db.session.commit", side_effect=SQLAlchemyError("db error")
):
with app.test_client() as client:
url = record_url(pid)
# TESTING=True propagates unhandled exceptions; catch at the
# test level and verify it is the original SQLAlchemyError, NOT
# a PIDResolveRESTError swallowing it.
with pytest.raises(SQLAlchemyError):
client.put(url, data=json.dumps(record.dumps()), headers=HEADERS)
Loading