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

Handle logging non-JSON-serializable classes in stream slices #22118

Merged
merged 3 commits into from
Jan 31, 2023

Conversation

erohmensing
Copy link
Contributor

What

Describe what the change is solving
It helps to add screenshots if it affects the frontend.
Avoid issues like these when trying to log stream slices which contain values that are not serializable. Note that in the case of the linked action, the streamslice class is not a Mapping, so it's not the most valid case here. However, when I implemented that as a TypedDict instead, I ran into the same issue for datetime formats, and I think they're a valid case we should support, hence that being the test case.

How

Describe the solution
When dumping, if a class is not json serializable, dump it as a string.

@erohmensing erohmensing requested a review from a team as a code owner January 30, 2023 23:35
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Jan 30, 2023
Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

This seems like a good idea. Out of curiosity, are there slices today where the value is not JSON serializable?

@maxi297
Copy link
Contributor

maxi297 commented Jan 31, 2023

Note that you will probably need to run SUB_BUILD=CONNECTORS_BASE ./gradlew format --scan --info --stacktrace

@erohmensing
Copy link
Contributor Author

erohmensing commented Jan 31, 2023

Yes! The run I linked is from a connector which defines streamslices as a StreamSlice dataclass (not a dictionary) with the following definition:

@dataclass
class StreamSlice:
    start_date: DateTime
    end_date: DateTime

obviously, this goes against typing to be using a data class instead of a dict or other mapping format - it should still probably be updated to be a TypedDict. However, if you fix that, then you still run into the error because start_date and end_date are pendulum.datetime.DateTimes, which are not json serializable.

I didn't notice they came from pendulum, not the datetime library itself, but when I wrote the failing test, datetime.date also failed for the same reason - TIL! But since stream_slices are Iterable[Optional[Mapping[str, Any]]], there could be datetimes or other custom objects in that Any.

@erohmensing
Copy link
Contributor Author

erohmensing commented Jan 31, 2023

And thanks, will format and add changelog info etc. so that I can release it :)

Edit: just remembered changelog and versioning are handled by the action, so convenient 💯

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

Successfully merging this pull request may close these issues.

3 participants