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

Fixes #1879: Serializes Sets in Response #1881

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leonelgalan
Copy link

I'm getting an item from a DynamoDB Table that has a set attribute.
Simply returning the item at the end of the request through Chalice
raises this error:

TypeError: Object of type set is not JSON serializable

Issue #1879

It handles set as an "extra" type during JSON Serialization.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

I'm getting an item from a DynamoDB Table that has a set attribute.
Simply returning the item at the end of the request through Chalice
raises this error:

TypeError: Object of type set is not JSON serializable
Copy link
Member

@jamesls jamesls 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 the PR. Not opposed to the change. I think the original reason for not supporting sets was to match what the default json serializer did in Lambda, but this seems reasonable to me, especially given DynamoDB can return sets.

Had a comment about the test you added, but once that's updated we should be good to merge.

@@ -1816,6 +1816,17 @@ def test_http_response_to_dict(body, headers, status_code):
assert isinstance(serialized['body'], six.string_types)


@given(headers=STR_MAP,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need these property based tests for this change. We're passing in varying headers and status code values but our test is just verifying the values of sets. You can probably just create various Response instances and verify the to_dict() serializes the types properly. Something like test_can_encode_binary_body_as_base64().

Probably worth adding an extra test for nested sets as well.

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

Successfully merging this pull request may close these issues.

2 participants