Skip to content

Conversation

@dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented Jul 25, 2025

Content

This PR includes the replacement of From implementations with fallible TryFrom for converting between Certificate and CertificateRecord.

This PR includes...

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • All check jobs of the CI have succeeded
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)
    • Add ADR blog post or Dev ADR entry (if relevant)
    • No new TODOs introduced

Issue(s)

Closes #2651

@dlachaume dlachaume self-assigned this Jul 25, 2025
@dlachaume dlachaume requested a review from Copilot July 25, 2025 14:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the conversion between Certificate and CertificateRecord types by replacing infallible From trait implementations with fallible TryFrom trait implementations. This change makes error handling explicit for conversions that could potentially fail during serialization/deserialization operations.

  • Replaced From implementations with TryFrom for CertificateCertificateRecord conversions
  • Updated all call sites to use try_into() with explicit error handling
  • Modified repository methods and test helpers to handle conversion errors properly

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
mithril-aggregator/src/database/record/certificate.rs Core implementation change from From to TryFrom for both conversion directions
mithril-aggregator/src/database/repository/certificate_repository.rs Updated repository methods to handle fallible conversions with proper error propagation
mithril-aggregator/src/database/test_helper.rs Modified test helper function to accept TryInto instead of Into
mithril-aggregator/src/tools/certificates_hash_migrator.rs Updated test utility functions to use try_into().unwrap()
mithril-aggregator/src/database/query/certificate/insert_certificate.rs Updated test code to use fallible conversions
mithril-aggregator/src/database/query/certificate/insert_or_replace_certificate.rs Updated test code to use fallible conversions
mithril-aggregator/src/database/query/certificate/get_certificate.rs Updated test code to use fallible conversions

@github-actions
Copy link

github-actions bot commented Jul 25, 2025

Test Results

    4 files  ±0    154 suites  ±0   22m 38s ⏱️ -10s
2 118 tests ±0  2 118 ✅ ±0  0 💤 ±0  0 ❌ ±0 
6 466 runs  ±0  6 466 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit dc4c8a5. ± Comparison against base commit 0122f3e.

♻️ This comment has been updated with latest results.

@dlachaume dlachaume temporarily deployed to testing-preview July 25, 2025 14:31 — with GitHub Actions Inactive
@dlachaume dlachaume force-pushed the dlachaume/2651/impl-try-from-certificate-and-certificate-record branch from c546db7 to 6226d17 Compare July 25, 2025 14:37
@dlachaume dlachaume temporarily deployed to testing-preview July 25, 2025 14:47 — with GitHub Actions Inactive
@dlachaume dlachaume temporarily deployed to testing-preview July 25, 2025 14:58 — with GitHub Actions Inactive
@dlachaume dlachaume requested review from Alenar and turmelclem July 25, 2025 14:59
Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

LGTM 🐕

Copy link
Collaborator

@turmelclem turmelclem left a comment

Choose a reason for hiding this comment

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

LGTM 🎸

@dlachaume dlachaume force-pushed the dlachaume/2651/impl-try-from-certificate-and-certificate-record branch from 0274f45 to 814b8b4 Compare July 28, 2025 08:33
* mithril-aggregator from `0.7.76` to `0.7.77`
* mithril-common from `0.6.11` to `0.6.12`
@dlachaume dlachaume force-pushed the dlachaume/2651/impl-try-from-certificate-and-certificate-record branch from 814b8b4 to dc4c8a5 Compare July 28, 2025 08:36
@dlachaume dlachaume temporarily deployed to testing-preview July 28, 2025 08:50 — with GitHub Actions Inactive
@dlachaume dlachaume merged commit 1069937 into main Jul 28, 2025
70 of 71 checks passed
@dlachaume dlachaume deleted the dlachaume/2651/impl-try-from-certificate-and-certificate-record branch July 28, 2025 09:01
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.

Convert from to try_from for Certificate and CertificateRecord

4 participants