Skip to content

fix(alerts): handle dictionary payload for crashlytics digestDate - #310

Open
hardikkaurani wants to merge 6 commits into
firebase:mainfrom
hardikkaurani:fix/stability-digest-parsing
Open

hardikkaurani wants to merge 6 commits into
firebase:mainfrom
hardikkaurani:fix/stability-digest-parsing

Conversation

@hardikkaurani

@hardikkaurani hardikkaurani commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR fixes Crashlytics alert payload deserialization when digestDate is formatted as a dictionary. It ensures stability digests can be parsed without throwing type errors.

Problem

Eventarc stability digest payloads for Crashlytics occasionally format digestDate as a dictionary instead of a string, causing the Python SDK's deserialization to fail.

Root Cause

The alerts module assumes digestDate will always be a primitive string, lacking defensive parsing for the dictionary representation.

Changes

  • Adds defensive dictionary parsing logic for digestDate in 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.

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

@google-cla

google-cla Bot commented Sep 25, 2026

Copy link
Copy Markdown

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.

@gemini-code-assist gemini-code-assist Bot 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.

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.

Comment thread src/firebase_functions/private/util.py Outdated
@hardikkaurani

Copy link
Copy Markdown
Contributor Author

@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!

@IzaakGough IzaakGough 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.

@hardikkaurani Thanks for picking this up, this looks good overall with just one point I think we need to address.

In real alerts, digestDate comes through as {"year": 2026, "month": 5, "day": 15} (see the payloads dumped in meditohq/medito-app#884). This branch finds no seconds and returns 1970-01-01 00:00:00+00:00, so handlers stop crashing but get the wrong date with no error. Could we parse that shape in stability_digest_payload_from_ce_payload, and raise an error on any dict we don't recognize instead of defaulting to 0?

@hardikkaurani

Copy link
Copy Markdown
Contributor Author

Just doing it.

…stamp dict format

- Parse Crashlytics Stability Digest calendar-date dict shape {year, month, day} in _alerts_fn._parse_digest_date into timezone-aware UTC datetime at midnight
- Prevent timestamp_conversion in util.py from defaulting missing seconds to 0 / Unix epoch; raise explicit ValueError on unrecognized or incomplete timestamp dicts
- Add comprehensive regression tests in tests/test_alerts_fn.py and tests/test_util.py
@hardikkaurani

Copy link
Copy Markdown
Contributor Author

Hi @IzaakGough,

Thanks for the review and pointing out the real alert payload shape from meditohq/medito-app#884!

I've updated the PR with commit 59d2271 to address your feedback:

  1. Calendar-date parsing in _alerts_fn.py: Added _parse_digest_date() to parse Crashlytics stability digest {"year": ..., "month": ..., "day": ...} calendar-date dictionaries into timezone-aware UTC datetime objects at midnight (00:00:00+00:00), rejecting None fields, missing fields, or out-of-range dates with an explicit ValueError.
  2. Prevent silent epoch fallbacks in util.py: Updated timestamp_conversion() to require valid seconds (no longer defaulting missing seconds to 0). Unrecognized, empty, or incomplete timestamp dictionaries now explicitly raise ValueError.
  3. Comprehensive regression tests: Added unit test coverage in tests/test_alerts_fn.py (covering valid calendar dates, timestamp dicts, ISO strings, missing fields, explicit None, out-of-range values, unrecognized dicts, and invalid types) and expanded tests/test_util.py to verify that invalid/unrecognized timestamp dictionaries raise ValueError.

All linters (ruff check, ruff format) pass cleanly. Please let me know if anything else is needed!

@hardikkaurani

Copy link
Copy Markdown
Contributor Author

@IzaakGough Review this one if you had a chance !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] stability_digest_payload_from_ce_payload fails with AttributeError: 'dict' object has no attribute 'split'

3 participants