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

check API version #641

Merged
merged 7 commits into from
Dec 19, 2022
Merged

check API version #641

merged 7 commits into from
Dec 19, 2022

Conversation

ghubertpalo
Copy link
Collaborator

@ghubertpalo ghubertpalo commented Dec 7, 2022

Content

This PR adds an API version check when requesting the Aggregator. If the API version does not match, the HTTP call is rejected.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • 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)

Comments

None

Issue(s)

Closes #633

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

Test Results

    3 files  ±  0    28 suites  ±0   2m 56s ⏱️ ±0s
355 tests +11  355 ✔️ +11  0 💤 ±0  0 ±0 
416 runs  +19  416 ✔️ +19  0 💤 ±0  0 ±0 

Results for commit b1504ae. ± Comparison against base commit e6c1527.

♻️ This comment has been updated with latest results.

@ghubertpalo ghubertpalo temporarily deployed to testing-preview December 12, 2022 17:55 — with GitHub Actions Inactive
@ghubertpalo ghubertpalo temporarily deployed to testing-preview December 13, 2022 13:23 — with GitHub Actions Inactive
@ghubertpalo ghubertpalo temporarily deployed to testing-preview December 13, 2022 15:32 — with GitHub Actions Inactive
.with(warp::reply::with::headers(headers))
}

/// API Version verification
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

change the comment according to what the function does

.untuple_one()
}

pub async fn handle_custom(reject: Rejection) -> Result<impl Reply, Rejection> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

handle custom rejection codes

.untuple_one()
}

pub async fn handle_custom(reject: Rejection) -> Result<impl Reply, Rejection> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not need to be public

@ghubertpalo
Copy link
Collaborator Author

Catch the HTTP error in the signer and the client to display an according error message.

@jpraynaud
Copy link
Member

We probably need to update the OpenAPI specification to add the new error code.

Also I'm not sure that we should use 412 HTTP code. Maybe 426 is more appropriate. WDYT?

@ghubertpalo ghubertpalo temporarily deployed to testing-preview December 14, 2022 19:18 — with GitHub Actions Inactive
@ghubertpalo ghubertpalo temporarily deployed to testing-preview December 19, 2022 09:02 — with GitHub Actions Inactive
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ghubertpalo ghubertpalo merged commit c625805 into main Dec 19, 2022
@ghubertpalo ghubertpalo deleted the greg/633/api_version branch December 19, 2022 09:28
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.

Enforcement of API Protocol versions in Client/Signer/Aggregator
2 participants