Skip to content

feat: revise OpenAPI compatibility enforcement#2572

Merged
dlachaume merged 12 commits into
mainfrom
dlachaume/2535/loose-enforcement-of-openapi-compatibility
Jun 16, 2025
Merged

feat: revise OpenAPI compatibility enforcement#2572
dlachaume merged 12 commits into
mainfrom
dlachaume/2535/loose-enforcement-of-openapi-compatibility

Conversation

@dlachaume

@dlachaume dlachaume commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Content

This PR removes the HTTP response status code 412 Precondition Failed returned by the aggregator on OpenAPI version mismatch and adds a client-side warning log when a mismatch is detected.

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 #2535

@dlachaume dlachaume self-assigned this Jun 13, 2025
@github-actions

github-actions Bot commented Jun 13, 2025

Copy link
Copy Markdown

Test Results

    3 files  ± 0     88 suites  ±0   13m 45s ⏱️ -38s
2 014 tests +15  2 014 ✅ +15  0 💤 ±0  0 ❌ ±0 
3 548 runs  +25  3 548 ✅ +25  0 💤 ±0  0 ❌ ±0 

Results for commit a72b8dd. ± Comparison against base commit bc83454.

This pull request removes 12 and adds 27 tests. Note that renamed tests count towards both.
mithril-aggregator ‑ http_server::routes::router::tests::test_bad_version
mithril-aggregator ‑ http_server::routes::router::tests::test_good_version
mithril-aggregator ‑ http_server::routes::router::tests::test_no_version
mithril-aggregator ‑ http_server::routes::router::tests::test_parse_version_error
mithril-aggregator ‑ services::aggregator_client::tests::test_epoch_settings_ko_412
mithril-client ‑ aggregator_client::tests::test_client_can_fallback_to_a_second_version_when_412_api_version_mismatch
mithril-client ‑ aggregator_client::tests::test_client_handle_412_api_version_mismatch_with_version_in_response_header
mithril-client ‑ aggregator_client::tests::test_client_handle_412_api_version_mismatch_without_version_in_response_header
mithril-signer ‑ services::aggregator_client::tests::test_aggregator_features_ko_412
mithril-signer ‑ services::aggregator_client::tests::test_epoch_settings_ko_412
…
mithril-aggregator ‑ services::aggregator_client::tests::warn_if_api_version_mismatch::test_does_not_log_or_fail_when_header_is_missing
mithril-aggregator ‑ services::aggregator_client::tests::warn_if_api_version_mismatch::test_does_not_log_or_fail_when_header_is_not_a_version
mithril-aggregator ‑ services::aggregator_client::tests::warn_if_api_version_mismatch::test_epoch_settings_ok_200_log_warning_if_api_version_mismatch
mithril-aggregator ‑ services::aggregator_client::tests::warn_if_api_version_mismatch::test_logs_error_when_aggregator_version_cannot_be_computed
mithril-aggregator ‑ services::aggregator_client::tests::warn_if_api_version_mismatch::test_logs_warning_when_leader_aggregator_api_version_is_newer
mithril-aggregator ‑ services::aggregator_client::tests::warn_if_api_version_mismatch::test_no_warning_logged_when_leader_aggregator_api_version_is_older
mithril-aggregator ‑ services::aggregator_client::tests::warn_if_api_version_mismatch::test_no_warning_logged_when_versions_match
mithril-client ‑ aggregator_client::tests::warn_if_api_version_mismatch::test_client_get_log_warning_if_api_version_mismatch
mithril-client ‑ aggregator_client::tests::warn_if_api_version_mismatch::test_client_post_log_warning_if_api_version_mismatch
mithril-client ‑ aggregator_client::tests::warn_if_api_version_mismatch::test_does_not_log_or_fail_when_header_is_missing
…

♻️ This comment has been updated with latest results.

@dlachaume
dlachaume temporarily deployed to testing-preview June 13, 2025 09:08 — with GitHub Actions Inactive
@dlachaume
dlachaume requested a review from Copilot June 13, 2025 09:08

Copilot AI left a comment

Copy link
Copy Markdown

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 revises the OpenAPI compatibility enforcement by eliminating the use of the 412 Precondition Failed response in the OpenAPI specification and shifting version mismatch handling to client-side warning logs.

  • Removed API version mismatch 412 responses from openapi.yaml.
  • Updated both signer and aggregator clients to log warnings instead of raising errors on version mismatches.
  • Adjusted tests and dependency configurations accordingly.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
openapi.yaml Removed 412 responses for API version mismatches.
mithril-signer/Cargo.toml Added semver dependency.
mithril-client/src/aggregator_client.rs Removed error handling for 412; added warning logging logic.
mithril-client/Cargo.toml Added dependency on http library.
mithril-aggregator/src/services/aggregator_client.rs Replaced error handling with warning log for version mismatch.
mithril-aggregator/src/http_server/routes/router.rs Removed API version header validation middleware.

Comment thread mithril-client/src/aggregator_client.rs

@Alenar Alenar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Comment thread mithril-aggregator/src/services/aggregator_client.rs Outdated
Comment thread mithril-aggregator/src/services/aggregator_client.rs Outdated

@jpraynaud jpraynaud left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

There is a lot of duplicated code, maybe it's time to create a common aggregator client which can be used in signer, aggregator and client?

Comment thread mithril-aggregator/src/services/aggregator_client.rs Outdated
Comment thread mithril-aggregator/src/services/aggregator_client.rs Outdated
Comment thread mithril-aggregator/src/services/aggregator_client.rs
Comment thread mithril-aggregator/src/services/aggregator_client.rs Outdated
Comment thread mithril-aggregator/src/services/aggregator_client.rs Outdated
Comment thread mithril-client/src/aggregator_client.rs Outdated
@dlachaume
dlachaume force-pushed the dlachaume/2535/loose-enforcement-of-openapi-compatibility branch 2 times, most recently from 3655419 to 7db9922 Compare June 13, 2025 13:59
@dlachaume
dlachaume temporarily deployed to testing-preview June 13, 2025 14:14 — with GitHub Actions Inactive
@dlachaume
dlachaume temporarily deployed to testing-preview June 13, 2025 15:03 — with GitHub Actions Inactive
Comment thread CHANGELOG.md Outdated
@dlachaume
dlachaume force-pushed the dlachaume/2535/loose-enforcement-of-openapi-compatibility branch from adc4f48 to 6be0ff8 Compare June 13, 2025 15:25
@dlachaume
dlachaume temporarily deployed to testing-preview June 13, 2025 15:36 — with GitHub Actions Inactive

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

LGTM

dlachaume added 12 commits June 16, 2025 10:50
- rewrite `API_VERSION_MISMATCH_WARNING_MESSAGE` value
- use tuple match instead of nested `if let` / `match` / `if` for better readability
- improve `warn!` usage
- enhance test readability
* mithril-aggregator from `0.7.61` to `0.7.62`
* mithril-client from `0.12.13` to `0.12.14`
* mithril-signer from `0.2.252` to `0.2.253`
* openapi.yaml from `0.1.49` to `0.1.50`
@dlachaume
dlachaume force-pushed the dlachaume/2535/loose-enforcement-of-openapi-compatibility branch from 6be0ff8 to a72b8dd Compare June 16, 2025 08:50
@dlachaume
dlachaume temporarily deployed to testing-preview June 16, 2025 08:59 — with GitHub Actions Inactive
@dlachaume
dlachaume merged commit 0bfc9fe into main Jun 16, 2025
38 of 51 checks passed
@dlachaume
dlachaume deleted the dlachaume/2535/loose-enforcement-of-openapi-compatibility branch June 16, 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.

Loose enforcement of OpenAPI compatibility

5 participants