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

fix(event-sources): handle dynamodb null type as none, not bool #929

Merged
merged 3 commits into from Dec 30, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Dec 27, 2021

Issue #, if available:

Description of changes:

Return for None for null types:

Example:

{
  "Records": [
    {
      ...
      "dynamodb": {
        "OldImage": {
          ...
          "Message": {
            "S": "New item!"
          },
          "Id": {
            "N": "101"
          }
        },
        "Keys": {
          "Id": {
            "N": "101"
          }
        },
        "NewImage": {
          ...
          "Message": {
            "NULL": true
          },
          "Id": {
            "N": "101"
          }
        },
      }
    }
  ]
}

Below null_value and get_value would have returned True, and now they return None.

from aws_lambda_powertools.utilities.data_classes import event_source
from aws_lambda_powertools.utilities.data_classes.dynamo_db_stream_event import (
    DynamoDBStreamEvent,
)

@event_source(data_class=DynamoDBStreamEvent)
def handler(event: DynamoDBStreamEvent, context):
    record = next(event.records)
    message_value = record.dynamodb.new_image.get("message")

    # get_value would be the preferred way to get the underlying value regardless of type
    assert message_value.get_value is None

    # Otherwise when you want to use `null_value` you should first change against `get_type`
    if message_value.get_type == AttributeValueType.Null:
        assert message_value.null_value is None

Checklist

Breaking change checklist

Before null_value was returning a boolean, True when NULL value was set to true and False when NULL value was not set (or was False)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 27, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2021

Codecov Report

Merging #929 (e49a858) into develop (cced6c4) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #929      +/-   ##
===========================================
- Coverage    99.88%   99.88%   -0.01%     
===========================================
  Files          118      118              
  Lines         5272     5271       -1     
  Branches       605      605              
===========================================
- Hits          5266     5265       -1     
  Misses           2        2              
  Partials         4        4              
Impacted Files Coverage Δ
...s/utilities/data_classes/dynamo_db_stream_event.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cced6c4...e49a858. Read the comment docs.

@heitorlessa heitorlessa changed the title fix(event-sources): return for none for null types fix(event-sources): handle dynamodb null type as none, not bool Dec 29, 2021
assert attribute_value.null_value == attribute_value.get_value


def test_dynamo_attribute_value_null_value_invalid():
example_attribute_value = {"NULL": False}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelbrewer - We are deserializing, {"NULL": False} should never happen (so should we raise a ValueError or just ignore that?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heitorlessa - i can remove that test, i just wanted it to be clear that we don't valid

    @property
    def null_value(self) -> None:
        """An attribute of type Null.

        Example:
            >>> {"NULL": True}
        """
        return None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which is the same as:

    def _deserialize_null(self, value):
        return None

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Question on whether NULL will ever be False, boto tests always caters for True

Type table only considers True so perhaps DynamoDB doesn't compute that: https://github.com/boto/boto3/blob/cf62d26cb0f28357c935d541ed3f0dae196cd26d/boto3/dynamodb/types.py#L85

@michaelbrewer
Copy link
Contributor Author

Question on whether NULL will ever be False, boto tests always caters for True

Type table only considers True so perhaps DynamoDB doesn't compute that: boto/boto3@cf62d26/boto3/dynamodb/types.py#L85

Because boto does not allow you to call _deserialize_null directly but we allow for it via null_value

@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 29, 2021
@heitorlessa heitorlessa added the bug Something isn't working label Dec 30, 2021
@heitorlessa heitorlessa merged commit f6db802 into aws-powertools:develop Dec 30, 2021
@heitorlessa heitorlessa deleted the fix-928 branch December 30, 2021 09:07
heitorlessa added a commit to ran-isenberg/aws-lambda-powertools-python that referenced this pull request Dec 31, 2021
…tools-python into complex

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python: (24 commits)
  docs: consistency around admonitions and snippets (aws-powertools#919)
  chore(deps-dev): bump mypy from 0.920 to 0.930 (aws-powertools#925)
  fix(event-sources): handle dynamodb null type as none, not bool (aws-powertools#929)
  fix(apigateway): support @app.not_found() syntax & housekeeping (aws-powertools#926)
  docs: Added GraphQL Sample API to Examples section of README.md (aws-powertools#930)
  feat(idempotency): support dataclasses & pydantic models payloads (aws-powertools#908)
  feat(tracer): ignore tracing for certain hostname(s) or url(s) (aws-powertools#910)
  feat(event-sources): cache parsed json in data class (aws-powertools#909)
  fix(warning): future distutils deprecation (aws-powertools#921)
  docs(batch): remove leftover from legacy
  docs(layer): bump Lambda Layer to version 6
  chore: bump to 1.23.0
  docs(apigateway): add new not_found feature (aws-powertools#915)
  docs: external reference to cloudformation custom resource helper (aws-powertools#914)
  feat(logger): allow handler with custom kwargs signature (aws-powertools#913)
  chore: minor housekeeping before release (aws-powertools#912)
  chore(deps-dev): bump mypy from 0.910 to 0.920 (aws-powertools#903)
  feat(batch): new BatchProcessor for SQS, DynamoDB, Kinesis (aws-powertools#886)
  fix(parser): overload parse when using envelope (aws-powertools#885)
  fix(parser): kinesis sequence number is str, not int (aws-powertools#907)
  ...
heitorlessa added a commit to huonw/aws-lambda-powertools-python that referenced this pull request Dec 31, 2021
…tools-python into feature/905-datetime

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python:
  feat(feature_flags): support beyond boolean values (JSON values) (aws-powertools#804)
  docs: consistency around admonitions and snippets (aws-powertools#919)
  chore(deps-dev): bump mypy from 0.920 to 0.930 (aws-powertools#925)
  fix(event-sources): handle dynamodb null type as none, not bool (aws-powertools#929)
  fix(apigateway): support @app.not_found() syntax & housekeeping (aws-powertools#926)
  docs: Added GraphQL Sample API to Examples section of README.md (aws-powertools#930)
  feat(idempotency): support dataclasses & pydantic models payloads (aws-powertools#908)
  feat(tracer): ignore tracing for certain hostname(s) or url(s) (aws-powertools#910)
  feat(event-sources): cache parsed json in data class (aws-powertools#909)
  fix(warning): future distutils deprecation (aws-powertools#921)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants