8000
Skip to content

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

Merged
Ella Rohm-Ensing (erohmensing) merged 3 commits into
masterfrom
ella/unserializable-cdk
Jan 31, 2023
Merged

Handle logging non-JSON-serializable classes in stream slices#22118
Ella Rohm-Ensing (erohmensing) merged 3 commits into
masterfrom
ella/unserializable-cdk

Conversation

@erohmensing
@erohmensing Ella Rohm-Ensing (erohmensing) commented Jan 30, 2023
Copy link
Copy Markdown
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.

@octavia-squidington-iv Octavia Squidington IV (octavia-squidington-iv) added the CDK Connector Development Kit label Jan 30, 2023
maxi297
Maxime Carbonneau-Leclerc (maxi297) approved these changes Jan 31, 2023
View reviewed changes
@maxi297 Maxime Carbonneau-Leclerc (maxi297) left a comment
Copy link
Copy Markdown
Contributor

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
Maxime Carbonneau-Leclerc (maxi297) commented Jan 31, 2023
Copy link
Copy Markdown
Contributor

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

@erohmensing
Ella Rohm-Ensing (erohmensing) commented Jan 31, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

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
Ella Rohm-Ensing (erohmensing) commented Jan 31, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

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 💯

@erohmensing
Ella Rohm-Ensing (erohmensing) deleted the ella/unserializable-cdk branch January 31, 2023 16:33
Closed
@github-actions github-actions Bot mentioned this pull request Feb 7, 2023
Closed
This was referenced Feb 8, 2023
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@maxi297 Maxime Carbonneau-Leclerc (maxi297) maxi297 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CDK Connector Development Kit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erohmensing @maxi297 @octavia-squidington-iv
0