Skip to content

Commit

Permalink
fix: allow CheckFailure with no failed indexes
Browse files Browse the repository at this point in the history
The denokv self-hosted implementation does not return indexes of failed
checks, so we need to allow this. The spec does not say that servers
MUST report indexes of failed checks, only that clients SHOULD report
failed indexes. See: denoland/denokv#110
  • Loading branch information
h4l committed Feb 3, 2025
1 parent 5e4a2ee commit c4bd891
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 28 deletions.
29 changes: 12 additions & 17 deletions src/denokv/datapath.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,18 +202,19 @@ class CheckFailure(DataPathDenoKvError):

all_checks: tuple[Check, ...]
"""All of the Checks sent with the AtomicWrite."""
failed_check_indexes: AbstractSet[int]
failed_check_indexes: AbstractSet[int] | None
"""
The indexes of Checks in all_checks keys whose versionstamp check failed.
The set is sorted with ascending iteration order.
The set is sorted with ascending iteration order. Will be None if the
database does not support reporting which checks failed.
"""

def __init__(
self,
message: str,
all_checks: Iterable[Check],
failed_check_indexes: Iterable[int],
failed_check_indexes: Iterable[int] | None,
*args: object,
endpoint: EndpointInfo,
) -> None:
Expand All @@ -222,12 +223,15 @@ def __init__(
self.all_checks = tuple(all_checks)
if len(self.all_checks) == 0:
raise ValueError("all_checks is empty")
ordered_indexes = sorted(failed_check_indexes)
if len(ordered_indexes) == 0:
raise ValueError("failed_check_indexes is empty")
if ordered_indexes[0] < 0 or ordered_indexes[-1] >= len(self.all_checks):

ordered_indexes = sorted(failed_check_indexes) if failed_check_indexes else []
if len(ordered_indexes) > 0 and (
ordered_indexes[0] < 0 or ordered_indexes[-1] >= len(self.all_checks)
):
raise IndexError("failed_check_indexes contains out-of-bounds index")
self.failed_check_indexes = {i: True for i in ordered_indexes}.keys()
self.failed_check_indexes = (
{i: True for i in ordered_indexes}.keys() if ordered_indexes else None
)


DataPathError: TypeAlias = Union[
Expand Down Expand Up @@ -539,15 +543,6 @@ async def atomic_write(
)
err.__cause__ = e
return Err(err)
except ValueError as e:
err = ProtocolViolation(
"Server responded to Data Path Atomic Write with "
"CHECK_FAILURE containing no failed checks",
data=write_output,
endpoint=endpoint,
)
err.__cause__ = e
return Err(err)
elif write_output.status == AtomicWriteStatus.AW_WRITE_DISABLED:
return Err(
EndpointNotUsable(
Expand Down
43 changes: 32 additions & 11 deletions test/test_datapath.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
from denokv._kv_values import VersionStamp
from denokv._pycompat.typing import Awaitable
from denokv._pycompat.typing import Callable
from denokv._pycompat.typing import Iterable
from denokv._pycompat.typing import Mapping
from denokv._pycompat.typing import Sequence
from denokv._pycompat.typing import TypeAlias
Expand Down Expand Up @@ -261,7 +262,10 @@ async def violation_atomic_write_check_failure_with_out_of_bounds_index(
).SerializeToString(),
)

async def violation_atomic_write_check_failure_without_failed_checks(
# The denokv self-hosted implementation does not return indexes of failed
# checks.
# https://github.com/denoland/denokv/issues/110
async def quirk_atomic_write_check_failure_without_failed_checks(
request: web.Request,
) -> web.Response:
write = AtomicWrite()
Expand Down Expand Up @@ -363,7 +367,7 @@ def add_datapath_post(app: web.Application, path: str, handler: Handler) -> None
)
app.router.add_post(
"/check_failure_without_failed_checks/atomic_write",
violation_atomic_write_check_failure_without_failed_checks,
quirk_atomic_write_check_failure_without_failed_checks,
)
app.router.add_post("/unusable/atomic_write", unusable_atomic_write)
app.router.add_post(
Expand Down Expand Up @@ -781,10 +785,12 @@ async def test_atomic_write__raises_when_given_endpoint_without_strong_consisten
),
(
"/check_failure_without_failed_checks",
lambda endpoint: ProtocolViolation(
"Server responded to Data Path Atomic Write with CHECK_FAILURE "
"containing no failed checks",
data=AtomicWriteOutput(status=AtomicWriteStatus.AW_CHECK_FAILURE),
lambda endpoint: CheckFailure(
"Not all checks required by the Atomic Write passed",
all_checks=[
Check(key=pack_key(("x",)), versionstamp=bytes(VersionStamp(0)))
],
failed_check_indexes=[],
endpoint=endpoint,
),
),
Expand Down Expand Up @@ -1385,6 +1391,26 @@ def test_CheckFailure(example_endpoint: EndpointInfo) -> None:
assert msg in str(e)


@pytest.mark.parametrize("failed_check_indexes", [None, ()])
def test_CheckFailure__failed_check_indexes_is_None_when_no_indexes(
failed_check_indexes: Iterable[int] | None, example_endpoint: EndpointInfo
) -> None:
checks = [
Check(key=bytes(KvKey(f"a{i}")), versionstamp=bytes(VersionStamp(i)))
for i in range(4)
]
# Failed_check_indexes can be empty (the self-hosted sqlite implementation
# does not return the indexes of failed checks).
e = CheckFailure(
"Foo",
all_checks=iter(checks),
failed_check_indexes=failed_check_indexes,
endpoint=example_endpoint,
)
assert e.all_checks == tuple(checks)
assert e.failed_check_indexes is None


def test_CheckFailure__validates_constructor_args(
example_endpoint: EndpointInfo,
) -> None:
Expand All @@ -1395,11 +1421,6 @@ def test_CheckFailure__validates_constructor_args(
"Foo", all_checks=[], failed_check_indexes=[], endpoint=example_endpoint
)

with pytest.raises(ValueError, match=r"failed_check_indexes is empty"):
CheckFailure(
"Foo", all_checks=checks, failed_check_indexes=[], endpoint=example_endpoint
)

with pytest.raises(
IndexError, match=r"failed_check_indexes contains out-of-bounds index"
):
Expand Down

0 comments on commit c4bd891

Please sign in to comment.