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

DynamoDB streaming binary content #6371

Merged
merged 4 commits into from Jul 4, 2022
Merged

DynamoDB streaming binary content #6371

merged 4 commits into from Jul 4, 2022

Conversation

giograno
Copy link
Member

This PR addresses #6364 where the user reported error messages in the logs while trying to stream the DDB changes to Kinesis. This was due to errors in serializing bytes content.
Introduced a new encoder for binary content (less broad than the CustomEncoder we already have) and introduced/refactored some tests.

        This PR addresses #6364, where the user found out about errors
        in dumping JSON while streaming dynamodb updates to kinesis.
        Introduced a specific encoder and wrote a test.
@giograno giograno temporarily deployed to localstack-ext-tests June 29, 2022 14:05 Inactive
@giograno giograno requested a review from whummer June 29, 2022 15:09
@giograno giograno marked this pull request as ready for review June 29, 2022 15:09
@github-actions
Copy link

github-actions bot commented Jun 29, 2022

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 10m 43s ⏱️ + 10m 38s
1 114 tests +3  1 075 ✔️ +4  39 💤  - 1  0 ±0 
1 425 runs  +7  1 357 ✔️ +6  68 💤 +1  0 ±0 

Results for commit 12f910f. ± Comparison against base commit 2706724.

♻️ This comment has been updated with latest results.

Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @giograno ! Added a small question/comment regarding binary data with non-printable characters..

localstack/utils/json.py Outdated Show resolved Hide resolved
stream_name = get_kinesis_stream_name(table_name)
wait_for_stream_ready(stream_name)
response = dynamodb_client.put_item(
TableName=table_name, Item={"id": {"S": "id1"}, "data": {"B": b"binary_data"}}
Copy link
Member

Choose a reason for hiding this comment

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

Curious what would happen if we add non-printable binary data here, e.g., "data": {"B": b"\x90"}. Added a small parity test in this PR which illustrates that this works (i.e., is a valid request) against real AWS.

Could we try adding this binary input data to this test here as well? This would be to ensure that the downstream logic does not fail when running to_str(..) on non-printable bytes.. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. to_str() was indeed failing for non-printable binaries. Added a errors="replace" while calling the decode method.

Co-authored-by: Waldemar Hummer <waldemar.hummer@gmail.com>
@giograno giograno temporarily deployed to localstack-ext-tests July 1, 2022 13:44 Inactive
@giograno giograno temporarily deployed to localstack-ext-tests July 1, 2022 14:03 Inactive
@giograno giograno requested a review from whummer July 1, 2022 14:05
@thrau thrau requested a review from alexrashed July 1, 2022 18:07
@thrau
Copy link
Member

thrau commented Jul 1, 2022

also added @alexrashed as reviewer since this relates to ASF

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

LGTM! Also, this shouldn't affect any ASF serializer or parser (since it basically only affects client calls and the parsers and serialziers traverse the hierarchy and pre-process individual fields depending on their type).

@giograno giograno merged commit eba8880 into master Jul 4, 2022
@giograno giograno deleted the dynamodb-binary-dump branch July 4, 2022 15:04
@localstack localstack locked and limited conversation to collaborators Jul 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants