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

Use a bespoke version of Either #2441

Merged
merged 8 commits into from
Mar 8, 2023
Merged

Use a bespoke version of Either #2441

merged 8 commits into from
Mar 8, 2023

Conversation

hlbarber
Copy link
Contributor

@hlbarber hlbarber commented Mar 8, 2023

Motivation and Context

https://docs.rs/tower/latest/tower/util/enum.Either.html

Both services must be of the same request, response, and error types.

This is not the case, the error types are not required to be the same - as seen in the actual Service implementation. The tower::util::Either instead takes either error and converts it to Box<dyn Error> which is not the intended behavior and conflicts with the contract of Smithy which wants either Infallible in the case of a HTTP middleware and Op::Error in the case of a model middleware.

Description

  • Roll bespoke version of Either which preserves error.
  • Add Plugin implementation for Plugin.
  • Use bespoke Either for FilterByOperationName.

@hlbarber hlbarber added bug Something isn't working server Rust server SDK labels Mar 8, 2023
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@hlbarber hlbarber marked this pull request as ready for review March 8, 2023 19:36
@hlbarber hlbarber requested review from a team as code owners March 8, 2023 19:36
@hlbarber hlbarber requested review from jjant and unexge March 8, 2023 19:36
@@ -119,6 +119,7 @@
//!

mod closure;
pub mod either;
Copy link
Contributor Author

@hlbarber hlbarber Mar 8, 2023

Choose a reason for hiding this comment

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

I think I should probably pull Either into this module actually, rather than pub mod.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would put Either in its own util module like tower does (given that Either is not just a Plugin, but a Service/Layer/Plugin combinator).

Copy link
Contributor Author

@hlbarber hlbarber Mar 8, 2023

Choose a reason for hiding this comment

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

Yeah that could work except all these Plugins are kinda utils though?

  • Stack
  • FilterByOperationName
  • OperationNameFn

The only non-util Plugin in this repo is the InstrumentPlugin which lives in a different module - src/instrument/plugin.rs.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

Copy link
Contributor

@jjant jjant left a comment

Choose a reason for hiding this comment

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

The current (tower 0.4.13) behaviour of tower's Either seems to be unintended, see this PR that changes it. It looks like we could use their type after that fix is released in 0.5 or 1.0, so let's keep an eye on that.

I think this is fine to merge in the meantime.

@@ -119,6 +119,7 @@
//!

mod closure;
pub mod either;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would put Either in its own util module like tower does (given that Either is not just a Plugin, but a Service/Layer/Plugin combinator).

rust-runtime/aws-smithy-http-server/src/plugin/either.rs Outdated Show resolved Hide resolved
@hlbarber hlbarber enabled auto-merge March 8, 2023 20:50
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@hlbarber hlbarber added this pull request to the merge queue Mar 8, 2023
Merged via the queue into main with commit b17a867 Mar 8, 2023
@hlbarber hlbarber deleted the harryb/better-either branch March 8, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants