Repository navigation
fix(alerts): handle dictionary payload for crashlytics digestDate - #310
hardikkaurani wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates the timestamp_conversion utility function to support dictionary-based timestamp inputs (containing seconds and nanoseconds) in addition to ISO 8601 strings, and adds corresponding unit tests. The review feedback highlights potential TypeError issues if dictionary keys are explicitly set to None, as well as potential floating-point precision issues when calculating fractional seconds. A robust alternative using datetime.timedelta and explicit None checks is suggested to address these concerns.
| if isinstance(time, dict): | ||
| seconds = int(time.get("seconds", 0)) | ||
| nanos = int(time.get("nanos", time.get("nanoseconds", 0))) | ||
| return _dt.datetime.fromtimestamp(seconds + (nanos / 1e9), tz=_dt.timezone.utc) |
There was a problem hiding this comment.
Using dict.get(key, default) only returns the default value if the key is completely absent from the dictionary. If the key is present but its value is explicitly None, it will return None, which would cause int(None) to raise a TypeError.
Additionally, adding seconds + (nanos / 1e9) can introduce floating-point precision/rounding issues when converting to a datetime object.
Following the standard pattern used in Google's official protobuf library for Python, we can safely handle potential None values and use datetime.timedelta with integer division (nanos // 1000) to avoid floating-point precision issues.
| if isinstance(time, dict): | |
| seconds = int(time.get("seconds", 0)) | |
| nanos = int(time.get("nanos", time.get("nanoseconds", 0))) | |
| return _dt.datetime.fromtimestamp(seconds + (nanos / 1e9), tz=_dt.timezone.utc) | |
| if isinstance(time, dict): | |
| seconds = int(time.get("seconds") if time.get("seconds") is not None else 0) | |
| nanos_val = time.get("nanos") | |
| if nanos_val is None: | |
| nanos_val = time.get("nanoseconds") | |
| nanos = int(nanos_val) if nanos_val is not None else 0 | |
| return _dt.datetime.fromtimestamp(seconds, tz=_dt.timezone.utc) + _dt.timedelta(microseconds=nanos // 1000) |
…ly handle None values
|
@IzaakGough @inlined Just checking in! The fix for Crashlytics digestDate dictionary parsing (#260) is fully implemented and tested. It properly falls back to dictionary deserialization when Eventarc payloads send objects instead of strings, preventing the SDK from crashing. All CI checks and CLA are green. Could you please take a look when you get a chance? Thanks! |
Summary
This PR fixes Crashlytics alert payload deserialization when
digestDateis formatted as a dictionary. It ensures stability digests can be parsed without throwing type errors.Problem
Eventarc stability digest payloads for Crashlytics occasionally format
digestDateas a dictionary instead of a string, causing the Python SDK's deserialization to fail.Root Cause
The
alertsmodule assumesdigestDatewill always be a primitive string, lacking defensive parsing for the dictionary representation.Changes
digestDatein the Crashlytics alert payload handling.Tests
Regression coverage is included for the affected behavior.
Why This Is Not a Duplicate
This PR addresses a different issue from #299.
alertsparsing, while feat: add support for VPC direct connect #299 patches global options and VPC Direct Connect.Therefore, the two PRs address independent problems and this change does not supersede or duplicate #299.
Scope
This change is limited to the Crashlytics alert payload parsing logic and does not alter other Eventarc handlers.
Original Template Items
Fixes #260