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.

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

Added force ttl check option to kv-table functions #725

Merged
merged 5 commits into from
Apr 6, 2023
Merged
Changes from 3 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
20 changes: 17 additions & 3 deletions global_helpers/panther_oss_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,21 @@ def kv_table() -> boto3.resource:
return _KV_TABLE


def get_counter(key: str) -> int:
def ttl_expired(response: dict) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

if we go with this approach you'll need to update all the ProjectionExpressions to also return the TTL. I see in a few spots we are only getting back a single column

"""Checks whether a response from the panther-kv table has passed it's TTL date"""
# This can be used when the TTL timing is very exacting and DDB's cleanup is too slow
expiration = response.get("Item", {}).get("expiresAt", 0)
return expiration and float(expiration) <= (datetime.now()).timestamp()


def get_counter(key: str, force_ttl_check: bool = False) -> int:
"""Get a counter's current value (defaulting to 0 if key does not exist)."""
response = kv_table().get_item(
Key={"key": key},
ProjectionExpression=_COUNT_COL,
)
if force_ttl_check and ttl_expired(response):
return 0
return response.get("Item", {}).get(_COUNT_COL, 0)


Expand Down Expand Up @@ -271,7 +280,7 @@ def put_dictionary(key: str, val: dict, epoch_seconds: int = None):
set_key_expiration(key, epoch_seconds)


def get_dictionary(key: str) -> dict:
def get_dictionary(key: str, force_ttl_check: bool = False) -> dict:
# Retrieve the item from DynamoDB
response = kv_table().get_item(Key={"key": key})

Expand All @@ -281,6 +290,9 @@ def get_dictionary(key: str) -> dict:
if not item:
return {}

if force_ttl_check and ttl_expired(response):
return {}

try:
# Deserialize from JSON to a Python dictionary
return json.loads(item)
Expand All @@ -291,12 +303,14 @@ def get_dictionary(key: str) -> dict:
) from exc


def get_string_set(key: str) -> Set[str]:
def get_string_set(key: str, force_ttl_check: bool = False) -> Set[str]:
"""Get a string set's current value (defaulting to empty set if key does not exit)."""
response = kv_table().get_item(
Key={"key": key},
ProjectionExpression=_STRING_SET_COL,
Copy link
Contributor

Choose a reason for hiding this comment

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

If i'm reading this correctly this will only pull back the stringset column and won't include the ttl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch, of course my live test was only on the dictionary one where that isn't the case 🤦 I was going to test the other two methods just for certainty's sake before merging just hadn't got to it yet!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darwayne do you think there is any downside to simply removing the ProjectionExpression parameter here, similar to how it works today in get_dictionary?

Copy link
Contributor

Choose a reason for hiding this comment

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

There could be some edge case where a user has a ton of data in some other column that they aren't trying to retrieve. So tradeoffs are mostly latency based

)
if force_ttl_check and ttl_expired(response):
return set()
return response.get("Item", {}).get(_STRING_SET_COL, set())


Expand Down