Skip to content

Commit

Permalink
Merge pull request #26 from xoeye/bugfix/presume-needs-prewrite-to-al…
Browse files Browse the repository at this point in the history
…low-proper-equality-comparison

fix: equality comparsion after using presume (make presume act more like a real get from DDB)
  • Loading branch information
Peter Gaultney authored Sep 30, 2021
2 parents b0b7681 + a8ec035 commit 73f3244
Show file tree
Hide file tree
Showing 9 changed files with 80 additions and 31 deletions.
16 changes: 16 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
### 1.15.1

Fixes to `versioned_transact_write_items`:

1. `presume` will now apply the standard dynamodb_prewrite transform,
so that we don't get into situations where tuples aren't equal to
lists, or floats aren't equal to Decimals, for instance. Anything
presumed should look like an actual DynamoDB value, in other
words. This oversight was leading to certain situations where
presume was being used causing a superfluous write to the table.
2. The returned transaction will always include any items inserted
into the transaction by `presume`, even if no effects were
applied. This only occurred when a single item was presumed for the
whole transaction and no changes were made to it, but it makes the
API slightly easier to work with.

## 1.15.0

Changes to `versioned_transact_write_items`:
Expand Down
55 changes: 33 additions & 22 deletions dev-utils/watch_ddb_stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@
DYNAMODB_STREAMS = make_dynamodb_stream_images_multicast()


def make_accept_stream_images(item_slicer: Callable[[dict], str]):
def accept_stream_item(images: ItemImages):
def make_accept_stream_images(item_slicer: Callable[[ItemImages], dict]):
def accept_stream_item(images: ItemImages) -> None:
old, new = images
if not old:
print(f"New item: {item_slicer(new)}") # type: ignore
print(f"New item: {item_slicer(images)}") # type: ignore
elif not new:
print(f"Deleted item {item_slicer(old)}")
print(f"Deleted item {item_slicer(images)}")
else:
print(f"Updated item; OLD: {item_slicer(old)} NEW: {item_slicer(new)}")
print(f"Updated item; DIFF: {item_slicer(images)}")

return accept_stream_item

Expand All @@ -34,7 +34,10 @@ def make_key_slicer(table):
except ValueError:
range_key = ""

def extract_primary_key(item: dict):
def extract_primary_key(images: ItemImages) -> dict:
old, new = images
item = new or old
assert item is not None
key = dict()
key[hash_key] = item[hash_key]
if range_key:
Expand All @@ -44,33 +47,41 @@ def extract_primary_key(item: dict):
return extract_primary_key


def make_item_slicer(key_slicer, attribute_names):
def item_slicer(images: ItemImages) -> dict:
old, new = images
if not new:
new = dict()
if not old:
old = dict()
item = new or old
key = key_slicer(images)
diff = {name for name in (set(old) | set(new)) if old.get(name) != new.get(name)}
return {
**key,
**{attr_name: item[attr_name] for attr_name in attribute_names if attr_name in item},
**{diff_name: item.get(diff_name) for diff_name in diff},
}

return item_slicer


def main():
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("table_name")
parser.add_argument(
"--attribute-names", help="Any attributes other than the key to print", nargs="*"
"--attribute-names",
help="Any attributes other than the key to print on every update; space separated",
nargs="*",
default=list(),
)
args = parser.parse_args()

DDB_RES = boto3.resource("dynamodb")

table = DDB_RES.Table(args.table_name)

if args.attribute_names:
key_slicer = make_key_slicer(table)

def item_slicer(item: dict):
return {
**key_slicer(item),
**{
attr_name: item[attr_name]
for attr_name in args.attribute_names
if attr_name in item
},
}

else:
item_slicer = make_key_slicer(table)
item_slicer = make_item_slicer(make_key_slicer(table), args.attribute_names)

try:
accept_stream_images = make_accept_stream_images(item_slicer)
Expand Down
4 changes: 2 additions & 2 deletions tests/xoto3/dynamodb/write_versioned/example_unit_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def create_and_link_task_unless_exists(t: wv.VersionedTransaction) -> wv.Version
# ^ put this item into this table as long as the item does not exist
user = user_table.require(dict(id=new_task["user_id"]))(t) # type: ignore
# ^ fetch the user and raise an exception if it does not exist
user["task_ids"].append(task_key["id"])
user["task_ids"] = (user.get("task_ids") or list()) + [task_key["id"]]
t = user_table.put(user)(t)
# ^ make sure that the user knows about its task
return t
Expand Down Expand Up @@ -63,7 +63,7 @@ def test_task_and_user_are_both_written_if_task_does_not_exist():
t = task_table.presume(task_key, None)(t)
# declare that the task does not exist - essentially this is a 'fixture', but without I/O
user_key = dict(id="steve")
user = dict(user_key, name="Actually Steve", task_ids=list())
user = dict(user_key, name="Actually Steve")
t = user_table.presume(user_key, user)(t)

t = create_and_link_task_unless_exists(t)
Expand Down
9 changes: 9 additions & 0 deletions tests/xoto3/dynamodb/write_versioned/run_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
VersionedTransaction,
delete,
get,
presume,
put,
require,
versioned_transact_write_items,
Expand Down Expand Up @@ -116,6 +117,14 @@ def transact_resource_not_found(**_):
)


def test_presumed_items_get_returned_even_without_effects_being_executed():
def noop(vt: VersionedTransaction) -> VersionedTransaction:
return presume(vt, "tableA", dict(id="presume_me"), None)

result_vt = versioned_transact_write_items(noop)
assert get(result_vt, "tableA", dict(id="presume_me")) is None


def test_integration_test_inc_and_create_and_delete(
integration_test_id_table, integration_test_id_table_put, integration_test_id_table_cleaner
):
Expand Down
6 changes: 6 additions & 0 deletions tests/xoto3/dynamodb/write_versioned/specify_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,9 @@ def test_define_table_doesnt_redefine():
tx = delete(VersionedTransaction(dict()), "table1", dict(id="foo"))
same_tx = define_table(tx, "table1", "id")
assert same_tx is tx


def test_presume_properly_performs_prewrite():
tx = VersionedTransaction(dict())
tx = presume(tx, "foo", dict(id="yo"), dict(id="yo", val=(1, 2, 3)))
assert require(tx, "foo", dict(id="yo")) == dict(id="yo", val=[1, 2, 3])
2 changes: 1 addition & 1 deletion xoto3/__about__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""xoto3"""
__version__ = "1.15.0"
__version__ = "1.15.1"
__author__ = "Peter Gaultney"
__author_email__ = "[email protected]"
6 changes: 3 additions & 3 deletions xoto3/dynamodb/write_versioned/lazy_batch_gets.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Collection, Mapping, Tuple
from typing import Collection, Mapping

from .errors import ItemUndefinedException
from .prepare import add_item_to_base_request, parse_batch_get_request, prepare_clean_transaction
Expand All @@ -10,7 +10,7 @@ def lazy_batch_getting_transaction_builder(
transaction_builder: TransactionBuilder,
item_keys_by_table_name: Mapping[str, Collection[ItemKey]],
batch_get_item: BatchGetItem,
) -> Tuple[VersionedTransaction, VersionedTransaction]:
) -> VersionedTransaction:
"""If your transaction attempts to get/require an item that was not
prefetched, we will stop, fetch it, and then retry your
transaction from the beginning.
Expand Down Expand Up @@ -38,7 +38,7 @@ def lazy_batch_getting_transaction_builder(
built_transaction = transaction_builder(clean_transaction)
if not undefined_needing_fetch:
# the builder didn't need to ask for anything
return clean_transaction, built_transaction
return built_transaction
else:
# the builder has asked for everything it knows to ask for
# and we need to restart from the batch get above.
Expand Down
6 changes: 3 additions & 3 deletions xoto3/dynamodb/write_versioned/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,12 @@ def versioned_transact_write_items(

built_transaction = None
for i, _ in enumerate(attempts_iterator or timed_retry()):
clean_transaction, built_transaction = lazy_batch_getting_transaction_builder(
built_transaction = lazy_batch_getting_transaction_builder(
transaction_builder, item_keys_by_table_name, batch_get_item,
)
if built_transaction is clean_transaction or _is_empty(built_transaction):
if _is_empty(built_transaction):
logger.info("No effects were defined, so the existing items will be returned as-is.")
return clean_transaction
return built_transaction

try:
transact_write_items(
Expand Down
7 changes: 7 additions & 0 deletions xoto3/dynamodb/write_versioned/specify.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
"""Low level API for specifying non-effect facts about the state of the database"""
from typing import Optional

from xoto3.dynamodb.prewrite import dynamodb_prewrite
from xoto3.utils.tree_map import SimpleTransform

from .ddb_api import table_name as _table_name
from .keys import hashable_key, standard_key_attributes
from .types import Item, ItemKey, TableNameOrResource, VersionedTransaction, _TableData
Expand All @@ -11,6 +14,8 @@ def presume(
table: TableNameOrResource,
item_key: ItemKey,
item_value: Optional[Item],
*,
prewrite_transform: Optional[SimpleTransform] = None,
) -> VersionedTransaction:

"""'To assume as true in the absence of proof to the contrary.'
Expand Down Expand Up @@ -56,6 +61,8 @@ def presume(
)
hkey = hashable_key(item_key)
if hkey not in table_data.items:
item_value = dynamodb_prewrite(item_value, prewrite_transform) if item_value else None
# this prewrite makes sure the value looks like it could have come out of DynamoDB.
return VersionedTransaction(
tables={
**transaction.tables,
Expand Down

0 comments on commit 73f3244

Please sign in to comment.