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

Remove Protocol enum #1829

Merged
merged 7 commits into from
Oct 10, 2022
Merged

Remove Protocol enum #1829

merged 7 commits into from
Oct 10, 2022

Conversation

hlbarber
Copy link
Contributor

@hlbarber hlbarber commented Oct 7, 2022

Motivation and Context

The Protocol enum defines a closed set of protocols supported by the rust-runtime crates. Its use within interfaces restricts extensibility.

While this is not a solution to #1703 it does progress towards the general goal.

Description

  • Remove Protocol enum from aws-smithy-http-server.
  • Remove dependence on Protocol across aws-smithy-http-server.
    • Use individual IntoResponse<P> on the RuntimeError.
    • Remove RuntimeError and rename RuntimeErrorKind to RuntimeError.
  • Remove dependence on Protocol across aws-smithy-http-server-python.
    • Middleware constructors now require a P with PyMiddlewareException: IntoResponse<P>. It is no longer fallible.
    • Remove PyApp::protocol method.

Notes

The most contentious changes occur within the aws-smithy-http-server-python changes.

@hlbarber hlbarber added server Rust server SDK needs-server-review python-server Python server SDK labels Oct 7, 2022
@hlbarber hlbarber marked this pull request as ready for review October 7, 2022 15:04
@hlbarber hlbarber requested a review from a team as a code owner October 7, 2022 15:04
{
Self {
handlers,
into_response: PyMiddlewareException::into_response,
Copy link
Contributor Author

@hlbarber hlbarber Oct 7, 2022

Choose a reason for hiding this comment

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

We store the into_response here as a fn to immediately erase the P type. This prevents any further generics in the parent structs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet!

"aws.protocols#awsjson10" => Protocol::AwsJson10,
"aws.protocols#awsjson11" => Protocol::AwsJson11,
_ => {
return Err(PyException::new_err(format!(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fallibility is gone. It's replaced by the bound PyMiddlewareException: IntoResponse<P> on the Layer::layer below. We can move it up to the constructor if we want the compile error more eagerly.


/// Tower [Layer] implementation of Python middleware handling.
///
/// Middleware stored in the `handlers` attribute will be executed, in order,
/// inside an async Tower middleware.
#[derive(Debug, Clone)]
pub struct PyMiddlewareLayer {
pub struct PyMiddlewareLayer<P> {
Copy link
Contributor Author

@hlbarber hlbarber Oct 7, 2022

Choose a reason for hiding this comment

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

TODO: Add documentation on what this P is. What is it, where do you find it etc?

@@ -63,8 +63,6 @@ pub trait PyApp: Clone + pyo3::IntoPy<PyObject> {

fn middlewares(&mut self) -> &mut PyMiddlewares;

fn protocol(&self) -> &'static str;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this because there's currently no method to go from struct AwsRestJson1 (etc) to "aws.protocols#restJson1". This can be easily added though if desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can simply write a code generated literal in the implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This protocol method in this trait was a hack to be honest. I like what you did here.

@hlbarber hlbarber requested a review from a team as a code owner October 7, 2022 16:54
@github-actions
Copy link

github-actions bot commented Oct 7, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Harry Barber added 3 commits October 7, 2022 17:26
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 7, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 7, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 7, 2022
Fix python tests

Restore default

Add missing codegenScope

Fix missing new::<P>
Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

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

I gave it a quick skim and lgtm Harry!

@crisidev
Copy link
Contributor

crisidev commented Oct 7, 2022

There are still some failure in CI, but they are trivial misses.

@github-actions
Copy link

github-actions bot commented Oct 7, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Oct 7, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber merged commit 78022d6 into main Oct 10, 2022
@hlbarber hlbarber deleted the harryb/remove-error-kind branch October 10, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python-server Python server SDK server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants