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 None check in deserialize.py #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

b-moynihan
Copy link

Avoids the scenario where NewImage is not found on the stream record resulting in an attempt to deserialize the items on NoneType ie. {'M': None} now calls as {'Null':True} instead.

Description of change

See issue #30 for details on fix

Manual QA steps

Configure DynamoDB Stream with StreamViewType : "KEYS_ONLY". Configure LOG_BASED Stitch extraction of DynamoDB stream.
When the extraction fails the following error message should be seen: Dynamo stream view type must be either "NEW_IMAGE" "NEW_AND_OLD_IMAGES NOT Extraction failed for ‘NoneType’ object has no attribute ‘items'

Risks

Rollback steps

  • revert this branch

Additional Info

This is actually a duplicate PR to #31 however this PR was closed as I deleted the linked Github account and didn't transfer the repo to my new account before hand.

…ge is not found on the stream record resulting in an attempt to deserialize the items on NoneType ie. `{'M': None}` now calls as `{'Null':True}` instead.
@cmerrick
Copy link
Contributor

Hi @b-moynihan, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@cmerrick
Copy link
Contributor

You did it @b-moynihan!

Thank you for signing the Singer Contribution License Agreement.

@b-moynihan
Copy link
Author

@cmerrick @KAllan357 @asaf-erlich @cosimon I've posted this PR to the tap-dynamodb slack channel however it is very quiet over there. So I'm also tagging you here to notify you of a pending review for this PR.

@asaf-erlich
Copy link
Contributor

cc @dmosorast

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants