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

Use type converter that cannot return null for date to avoid crash #1573

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Nov 29, 2023

Description

This PR is adding a type converter that never returns null to address a crash we've seen a sudden increase in on Sentry.

The root of the crash seems to be that a null Long is being retrieved from the podcast_episodes database table. This should never happen because that column is not allowed to be null. This is the generated code that is leading to the error:

if (_cursor.isNull(_cursorIndexOfPublishedDate)) {
  _tmp = null;
} else {
  _tmp = _cursor.getLong(_cursorIndexOfPublishedDate);
}
final Date _tmp_1 = __dateTypeConverter.toDate(_tmp);
if(_tmp_1 == null) {
  throw new IllegalStateException("Expected non-null java.util.Date, but it was null.");
} else {
  _tmpPublishedDate = _tmp_1;
}

This PR adds a SafeDate typealias for the Date class along with a TypeConverter that will never return null and therefore handles providing valid Date objects even if it is passed a null. I'm doing this instead of updating the current DateTypeConverter because we need the type converter to be able to return null dates #1390 (comment). This is what the generated code looks like with the new type converter:

final Date _tmpPublishedDate;
final long _tmp;
_tmp = _cursor.getLong(_cursorIndexOfPublishedDate);
final Long _tmp_1;
_tmp_1 = _tmp;      // No longer checking for null here because the type converter never returns null values
_tmpPublishedDate = __shouldNotBeNullDateTypeConverter.toDate(_tmp_1);

Fixes #1572

Testing Instructions

I have not found a way to directly reproduce this error in an unaltered app, so let's do two tests, one with the unaltered app, and one with an altered build that forces the app to handle a null value from the database.

1. Unaltered app

  1. Go to the podcast page for "The Daily" podcast
  2. Observe that everything works as it should
  3. Go to the podcast page for a few other podcasts, checking back on "The Daily" podcast a few times
  4. Observe that they also work without any issues.

2. Force the type converter to handle a null value

  1. Alter the ShouldNotBeNullDateTypeConverter::toDate method so that it always returns Date(Instant.EPOCH.toEpochMilli()) (i.e., what it would return if null was passed to the method).
  2. Build the altered app and deploy it to your device
  3. Go to the podcast page for "The Daily" podcast
  4. Observe that all of the episodes have a date of "December 31, 1969" but that otherwise everything works as it should
  5. Go to the podcast page for a few other podcasts
  6. Observer that they also work within any issues apart from all the episodes having the date epoch date.

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@mchowning mchowning marked this pull request as ready for review November 29, 2023 20:54
@mchowning mchowning requested a review from a team as a code owner November 29, 2023 20:54
@mchowning mchowning added the [Type] Bug Not functioning as intended. label Nov 29, 2023
@mchowning mchowning added this to the 7.53 ❄️ milestone Nov 29, 2023
@pocketcasts
Copy link

1 Warning
⚠️ This PR is assigned to the milestone 7.53 ❄️. The due date for this milestone has already passed.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@mchowning mchowning changed the title Use type converter that cannot return null for non-null date to avoid crash Use type converter that cannot return null for date to avoid crash Nov 29, 2023
Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ashiagr ashiagr merged commit 0df5efe into release/7.53 Nov 30, 2023
10 checks passed
@ashiagr ashiagr deleted the fix/null-date-crash branch November 30, 2023 04:34
mchowning added a commit that referenced this pull request Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Not functioning as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants