Skip to content

Kotlin local date#1154

Open
david-livefront wants to merge 2 commits into
mainfrom
kotlin-lacal-date
Open

Kotlin local date#1154
david-livefront wants to merge 2 commits into
mainfrom
kotlin-lacal-date

Conversation

@david-livefront
Copy link
Copy Markdown
Collaborator

🎟️ Tracking

N/A

📔 Objective

This is my first SDK PR and Claude helped me manage a lot of the heavy lifting, please let me know if I am properly honoring the best practices.

This PR updates the date values in the SDK for some of the new cipher types to utilize the Rust NaiveDate and translate it to a Java LocalDate.

🚨 Breaking Changes

This will create a breaking change for the Kotlin output of the SDK where previously certain date Strings will now be converted to the type-safe LocalDate class. This is required to enforce a consistent date format that can be parsed an utilized properly across the platforms.

@david-livefront david-livefront requested review from a team as code owners June 1, 2026 21:54
@david-livefront david-livefront added the ai-review-vnext Request a Claude code review using the vNext workflow label Jun 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR converts date_of_birth, issue_date, and expiration_date on PassportView and DriversLicenseView from Option<String> to Option<chrono::NaiveDate>, wires the type through the UniFFI custom-type machinery (mapped to Kotlin java.time.LocalDate), exposes it as an ISO 8601 string in WASM/TypeScript, and updates the CipherBlobV1 round-trip conversions plus tests. The cryptographic wire format is preserved (the encrypted payload is still a String), and the existing test_recorded_*_test_vector backwards-compatibility tests continue to pass, so existing encrypted vaults remain decryptable.

The implementation follows the patterns established by the sibling DateTime/Uuid UniFFI custom types. Replacing impl_bidirectional_from! with hand-written From impls in the blob conversions is reasonable now that field-level mapping is needed for the date fields, and the rest of the field mapping mirrors the macro behavior. No new findings beyond the existing review thread.

Code Review Details
  • Existing thread (crates/bitwarden-vault/src/cipher/passport.rs:97-102) already covers the silent coercion to None on NaiveDate::from_str failure in both Decryptable impls and the blob From<&*DataV1> conversions. No additional findings.

Comment on lines +91 to +96
date_of_birth: self
.date_of_birth
.decrypt(ctx, key)
.ok()
.flatten()
.and_then(|s: String| s.parse().ok()),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

QUESTION: Silent data loss when the decrypted date string is not a valid ISO 8601 date.

Details

.and_then(|s: String| s.parse().ok()) discards the value when parsing fails, so any existing encrypted data containing a non-NaiveDate-parseable string (e.g. "06/15/1985", "June 15, 1990", an empty string, or any locale-specific format a prior client may have stored before this type-safety pass) decrypts successfully but appears as None in the PassportView. The encrypted string is still on the server, but the user-facing field silently empties.

This same pattern repeats for issue_date (109-114) and expiration_date (115-120), and again in DriversLicense::decrypt and the blob From<&PassportDataV1> / From<&DriversLicenseDataV1> conversions.

Given these cipher types are recent (commit e504da35) and clients have not yet shipped UIs that write to them, real-world impact is likely small — but two questions are worth confirming before merge:

  1. Is silent coercion to None on parse failure the desired behavior, or should decrypt surface a CryptoError (or at minimum emit a tracing::warn!) so non-ISO data is observable rather than disappearing?
  2. The existing test_recorded_*_test_vector tests assert backwards-compat for the encrypted-string format, but they only cover values that already happen to be ISO. Worth a unit test asserting the chosen behavior for a non-parseable decrypted string so the contract is locked in.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 1, 2026

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

🔍 SDK Breaking Change Detection

SDK Version: kotlin-lacal-date (afce2d3)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
android ❌ Breaking changes detected Compilation failed with new SDK version. A corresponding pull request addressing the breaking changes must be ready for merge in bitwarden/android. - View Details
typescript ✅ No breaking changes detected Compilation passed with new SDK version - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 99.22481% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.14%. Comparing base (7bbd062) to head (8605774).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-core/src/uniffi_support.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1154      +/-   ##
==========================================
+ Coverage   84.12%   84.14%   +0.02%     
==========================================
  Files         446      446              
  Lines       58817    58912      +95     
==========================================
+ Hits        49478    49572      +94     
- Misses       9339     9340       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

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

Whether NaiveData is the correct usage here or not will have to be up to Vault (my understanding is that timezone data will be lost), but the bigger hurdle for this PR is WASM support. You'll have to confirm what type of object NaiveDate is translated into on the TypeScript side and then add a custom typescript section for it, similar to how DateTime looks (search for it in the code to find where to put it:

/**
 * RFC3339 compliant date-time string.
 * @typeParam T - Not used in JavaScript.
 */
export type DateTime<T = unknown> = string;

Most of the changes seem to be within Vaults domain so make sure to reach out to them for help and guidance!

Copy link
Copy Markdown
Contributor

@nick-livefront nick-livefront left a comment

Choose a reason for hiding this comment

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

Whether NaiveData is the correct usage here or not will have to be up to Vault (my understanding is that timezone data will be lost)

These specific dates do not have timezone data, they're only YYYY-MM-DD dates.

I do agree with the custom types approach that @coroiu suggested as the NaiveDate gets converted to a string on the TS side. The other option I considered was adding #[cfg_attr(feature = "wasm", tsify(type = "string"))] to each property but that would be tedious to maintain.

I'll send this to our internal channel to see if there are any other opinions but I think you could continue with the custom type.

@david-livefront david-livefront changed the title Kotlin lacal date Kotlin local date Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow breaking-change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants