Skip to content

Added test and fix for wrong time format and serialization in events v2 #11959

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
Merged
merged 3 commits into from
Nov 29, 2024
Merged

Added test and fix for wrong time format and serialization in events v2 #11959

merged 3 commits into from
Nov 29, 2024

Conversation

zaingz
Copy link
Contributor

@zaingz zaingz commented Nov 28, 2024 •
edited
Loading

Motivation

Found through an issue: #11630
When using EventBridge v2, events containing datetime fields fail to be delivered due to JSON serialization error: Object of type datetime is not JSON serializable. This affects events with explicit Time fields as well as automatically generated timestamps.

Changes

  • Added format_event_time function to properly format datetime objects to ISO8601 strings
  • Updated format_event to use the formatted time string
  • Added test to verify datetime serialization and delivery behavior matches AWS

The fix ensures proper datetime handling in events while maintaining AWS parity by formatting all time values to ISO8601 strings (YYYY-MM-DDThh:mm:ssZ).

@zaingz zaingz self-assigned this Nov 28, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 28, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 7m 50s ⏱️ - 41m 6s
2 237 tests  - 1 538  2 107 ✅  - 1 322  130 💤  - 216  0 ❌ ±0 
2 239 runs   - 1 538  2 107 ✅  - 1 322  132 💤  - 216  0 ❌ ±0 

Results for commit 5610d08. ± Comparison against base commit 892eb4d.

This pull request removes 1541 and adds 3 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.events.test_events.TestEvents ‑ test_put_events_with_time_field
tests.aws.services.stepfunctions.v2.error_handling.test_task_lambda.TestTaskLambda ‑ test_raise_custom_exception
tests.aws.services.stepfunctions.v2.error_handling.test_task_service_lambda.TestTaskServiceLambda ‑ test_raise_custom_exception

♻️ This comment has been updated with latest results.

@zaingz zaingz marked this pull request as ready for review November 29, 2024 10:04
@zaingz zaingz added the semver: patch Non-breaking changes which can be included in patch releases label Nov 29, 2024
maxhoheiser
maxhoheiser approved these changes Nov 29, 2024
View reviewed changes
Copy link
Member

@maxhoheiser maxhoheiser left a comment

Choose a reason for hiding this comment

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

LGTMT - nice implementation - I belive we had a custom json encoder at one point for serialization

maxhoheiser
maxhoheiser reviewed Nov 29, 2024
View reviewed changes
localstack-core/localstack/aws/api/events/__init__.py Outdated
@@ -919,7 +919,7 @@ class EventSource(TypedDict, total=False):


EventSourceList = List[EventSource]
EventTime = datetime
EventTime = Union[datetime, str]
Copy link
Member

Choose a reason for hiding this comment

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

maybe use the newer recommended way datetime | str

maxhoheiser
maxhoheiser reviewed Nov 29, 2024
View reviewed changes
localstack-core/localstack/services/events/utils.py Outdated
@@ -130,6 +130,11 @@ def get_event_time(event: PutEventsRequestEntry) -> EventTime:
return event_time


def format_event_time(event_time: datetime) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

why would we need the same function 2 times? just use event_time_to_time_string ?

@zaingz zaingz mentioned this pull request Nov 29, 2024
Open
1 task
@zaingz zaingz merged commit a47e701 into master Nov 29, 2024
32 checks passed
@zaingz zaingz deleted the events/fix/date-not-serializable branch November 29, 2024 12:14
@bentsku bentsku added the aws:events Amazon EventBridge label Nov 29, 2024
@bentsku bentsku added this to the 4.0.3 milestone Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxhoheiser maxhoheiser maxhoheiser approved these changes

@joe4dev joe4dev Awaiting requested review from joe4dev

Assignees

@zaingz zaingz

Labels
aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@zaingz @maxhoheiser @bentsku