Replace ZonedDateTime with Instant#6554
Conversation
|
|
||
| @ColumnInfo(name = "date") | ||
| val date: ZonedDateTime, | ||
| val date: Instant, |
There was a problem hiding this comment.
The schema does not need to be updated because we have a TypeConvertor that is transforming the value to a Long. So the stored datatype has not actually changed.
|
Claude finished @david-livefront's task in 4m 20s —— View job Pull Request Review: Replace ZonedDateTime with InstantSummaryThis PR replaces The migration is clean and mechanically correct. Key areas verified:
FindingsNo issues found. The migration is consistent and correct across all changed files. Re-reviewed after rebase onto latest main. No substantive code changes from the previous review. |
| reprompt = reprompt.toNetworkRepromptType(), | ||
| passwordHistory = passwordHistory?.toEncryptedNetworkPasswordHistoryList(), | ||
| lastKnownRevisionDate = ZonedDateTime.ofInstant(revisionDate, ZoneOffset.UTC), | ||
| lastKnownRevisionDate = revisionDate, |
There was a problem hiding this comment.
Little conversions like this are just not needed anymore.
|
Great job! No new security vulnerabilities introduced in this pull request |
network/src/main/kotlin/com/bitwarden/network/api/UnauthenticatedIdentityApi.kt
Outdated
Show resolved
Hide resolved
| override fun deserialize(decoder: Decoder): Instant = | ||
| ZonedDateTime | ||
| .parse(decoder.decodeString(), dateTimeFormatterDeserialization) | ||
| .toInstant() |
There was a problem hiding this comment.
This is one of the few remaining places that still uses the ZonedDateTime.
I did test out using Instant.parse(decoder.decodeString()) and did not see any negative ramification of the change but I am unsure of a good way to validate that it works for any and all variations the server could send back to us.
So for the moment, I have left it as a ZonedDateTime, which we know works, and then convert it to an Instant.
There was a problem hiding this comment.
Any concerns with migrating directly to Instant with this:
override fun deserialize(decoder: Decoder): Instant = Instant.parse(decoder.decodeString())There was a problem hiding this comment.
I like that ZonedDateTime is more permissive. I'm assuming the DateTimeFormatter pattern accepts . or : as a separator for a reason, and if : is received, Instant.parse() is going to fail. ZonedDateTime also handles various timezone offset formats more gracefully.
If we're not able to guarantee the server's format now and in the future, ZonedDateTime feels like the safer approach. The flexibility is worth the couple of extra lines, imo.
There was a problem hiding this comment.
Fair enough, we will run with this for now.
3362c29 to
964bf6b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6554 +/- ##
==========================================
- Coverage 86.45% 86.41% -0.04%
==========================================
Files 786 802 +16
Lines 56712 57144 +432
Branches 8258 8255 -3
==========================================
+ Hits 49028 49380 +352
- Misses 4817 4904 +87
+ Partials 2867 2860 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
964bf6b to
13b18e8
Compare
|
Thanks @SaintPatrck |

🎟️ Tracking
N/A
📔 Objective
This PR replaces the usage of
ZonedDateTimewithInstant. All the dates in the app are in UTC already and the Bitwarden SDK uses Instant as well. This should simplify logic within the app and remove the need to convert class types when interacting with the SDK.