Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(event_sources): implement Mapping protocol on DictWrapper for better interop with existing middlewares #1516

Merged
merged 2 commits into from
Sep 28, 2022
Merged

fix(event_sources): implement Mapping protocol on DictWrapper for better interop with existing middlewares #1516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5394f5
ISSUE-1503: ISSUE-1503: ISSUE-1503: fix: add Mapping abc and missing …
Tankanow Sep 12, 2022
9875e22
Apply suggestions from code review
Tankanow Sep 28, 2022
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
11 changes: 9 additions & 2 deletions aws_lambda_powertools/utilities/data_classes/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import base64
import json
from typing import Any, Dict, Optional
from collections.abc import Mapping
from typing import Any, Dict, Iterator, Optional


class DictWrapper:
class DictWrapper(Mapping):
Copy link
Contributor Author

@Tankanow Tankanow Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: I thought for a long time whether or not to explicitly add the Mapping protocol here or to simply implement the missing dunder methods and allow duck-typing to carry the day.

In the end, I decided it was better to explicitly add the Mapping protocol because, all things being equal, I think it is bad for subclasses of this class to break that protocol. It is good for mypy to warn when an implementer chooses to break the protocol. It should require an explicit type: ignore comment when choosing to break the protocol.

There is one existing class, StreamRecord, that breaks the protocol. There are no subclasses of StreamRecord and it is used only within the context of a DynamoDBRecord object. Because there is only one instance of this type breakage, and it is so localized, I marked StreamRecord.keys as type: ignore and added a comment and some tests to indicate that this class explicitly breaks the Mapping protocol.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In older days I'd ask to remove it as we're breaking it in StreamRecord - I'd take this as an instance of Practicality Beats Purity.

It's highly unlikely a customer will use .keys to get the keys of a StreamRecord dict given the intent. However, if we do receive any customer complaint we'll consider removing the subclassing.

"""Provides a single read only access to a wrapper dict"""

def __init__(self, data: Dict[str, Any]):
Expand All @@ -19,6 +20,12 @@ def __eq__(self, other: Any) -> bool:

return self._data == other._data

def __iter__(self) -> Iterator:
return iter(self._data)

def __len__(self) -> int:
return len(self._data)

def get(self, key: str, default: Optional[Any] = None) -> Optional[Any]:
return self._data.get(key, default)

Expand Down
4 changes: 3 additions & 1 deletion aws_lambda_powertools/utilities/data_classes/dynamo_db_stream_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,10 @@ def approximate_creation_date_time(self) -> Optional[int]:
item = self.get("ApproximateCreationDateTime")
return None if item is None else int(item)

# NOTE: This override breaks the Mapping protocol of DictWrapper, it's left here for backwards compatibility with
# a 'type: ignore' comment. See #1516 for discussion
@property
def keys(self) -> Optional[Dict[str, AttributeValue]]:
def keys(self) -> Optional[Dict[str, AttributeValue]]: # type: ignore[override]
"""The primary key attribute(s) for the DynamoDB item that was modified."""
return _attribute_value_dict(self._data, "Keys")

Expand Down
31 changes: 31 additions & 0 deletions tests/functional/test_data_classes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
AttributeValueType,
DynamoDBRecordEventName,
DynamoDBStreamEvent,
StreamRecord,
StreamViewType,
)
from aws_lambda_powertools.utilities.data_classes.event_source import event_source
Expand Down Expand Up @@ -101,6 +102,19 @@ def message(self) -> str:
assert DataClassSample(data1).raw_event is data1


def test_dict_wrapper_implements_mapping():
class DataClassSample(DictWrapper):
pass

data = {"message": "foo1"}
event_source = DataClassSample(data)
assert len(event_source) == len(data)
assert list(event_source) == list(data)
assert event_source.keys() == data.keys()
assert list(event_source.values()) == list(data.values())
assert event_source.items() == data.items()


def test_cloud_watch_dashboard_event():
event = CloudWatchDashboardCustomWidgetEvent(load_event("cloudWatchDashboardEvent.json"))
assert event.describe is False
Expand Down Expand Up @@ -617,6 +631,23 @@ def test_dynamo_attribute_value_type_error():
print(attribute_value.get_type)


def test_stream_record_keys_with_valid_keys():
attribute_value = {"Foo": "Bar"}
record = StreamRecord({"Keys": {"Key1": attribute_value}})
assert record.keys == {"Key1": AttributeValue(attribute_value)}


def test_stream_record_keys_with_no_keys():
record = StreamRecord({})
assert record.keys is None


def test_stream_record_keys_overrides_dict_wrapper_keys():
data = {"Keys": {"key1": {"attr1": "value1"}}}
record = StreamRecord(data)
assert record.keys != data.keys()


def test_event_bridge_event():
event = EventBridgeEvent(load_event("eventBridgeEvent.json"))

Expand Down