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.

Sign up for GitHub

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

Jump to bottom

Add request ID to response headers #2438

Merged
merged 24 commits into from
Mar 23, 2023
Merged

Add request ID to response headers #2438

merged 24 commits into from
Mar 23, 2023

Conversation

82marbag
Copy link
Contributor

@82marbag 82marbag commented Mar 7, 2023

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@82marbag 82marbag requested a review from a team as a code owner March 7, 2023 11:10
@82marbag 82marbag requested a review from a team as a code owner March 7, 2023 11:59
@smithy-lang smithy-lang deleted a comment from github-actions bot Mar 7, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Mar 7, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Mar 7, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Mar 7, 2023
@github-actions
Copy link

github-actions bot commented Mar 7, 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.

@82marbag 82marbag force-pushed the request-id-response-headers branch 2 times, most recently from 0b817b9 to 147eef2 Compare March 13, 2023 13:27
@82marbag
Add request ID to response headers
fdf1f2d 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Add parsing test
e6dcb1d 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Style
29ea8fe 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
82marbag and others added 11 commits March 22, 2023 12:26
@82marbag
CHANGELOG
61a1a15 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Fix import
0c8f3a1 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Panic if ServerRequestIdProviderLayer is not present
d12cbb1 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Own value
265be65 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Correct docs
0b98fd8 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Add order of layer to expect() message
f75de9e 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Remove Box
c24b86b 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Require order of request ID layers
ae9658d 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Revert "Require order of request ID layers"
64c2acd 
This reverts commit 147eef2.
@82marbag
One layer to generate and inject the header
1da38bb 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
HeaderName for header name
75abca7 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
CHANGELOG
67e8d8f 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Remove additional layer
33bffbf 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Remove to_owned
2910853 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Add tests, remove unnecessary clone
38d0302 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
take() ResponsePackage instead
a244b9d 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag 82marbag force-pushed the request-id-response-headers branch from 733701a to a244b9d Compare March 22, 2023 11:42
hlbarber
hlbarber reviewed Mar 22, 2023
View reviewed changes
CHANGELOG.next.toml Outdated
```
"""
references = ["smithy-rs#2438"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "server"}
Copy link
Contributor

@hlbarber hlbarber Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Technically, this is a breaking change:

- type Future = S::Future;
+ type Future = ServerRequestIdResponseFuture<S::Future>;

@82marbag
Update docs
842d0cc 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@github-actions
Copy link

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.

LukeMathWalker
LukeMathWalker reviewed Mar 22, 2023
View reviewed changes
rust-runtime/aws-smithy-http-server/src/request/request_id.rs Outdated
Comment on lines 20 to 22
//! To optionally add the [`ServerRequestId`] to the response headers, use [`ServerRequestIdProviderLayer::new_with_response_header`].
//! Use [`ServerRequestIdProviderLayer::new`] to use [`ServerRequestId`] in your handler.
//! Use [`ServerRequestIdProviderLayer::new_with_response_header`] to use [`ServerRequestId`] in your handler and add it to the response headers.
Copy link
Contributor

Choose a reason for hiding this comment

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

We speak of the fact that the server request id can be returned to the caller a couple of paragraphs above, I think it makes sense to mention the relevant constructor there.

LukeMathWalker
LukeMathWalker reviewed Mar 22, 2023
View reviewed changes
rust-runtime/aws-smithy-http-server/src/request/request_id.rs Outdated
pub struct ServerRequestIdProviderLayer;
pub struct ServerRequestIdProviderLayer {
header_key: Option<HeaderName>,
}

impl ServerRequestIdProviderLayer {
/// Generate a new unique request ID
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd update this to mention this other constructor and explain that it won't add it to the response headers (and vice versa in the docs for the other method).

LukeMathWalker
LukeMathWalker approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@LukeMathWalker LukeMathWalker left a comment

Choose a reason for hiding this comment

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

A couple of docs comments, good to go otherwise!

@82marbag
Update docs
24f58a3 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@github-actions
Copy link

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.

@82marbag
cargo fmt
5dd2670 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Update CHANGELOG
c41ba44 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Merge branch 'main' into request-id-response-headers
4f7e150
@82marbag 82marbag enabled auto-merge March 22, 2023 16:36
@82marbag 82marbag disabled auto-merge March 22, 2023 16:42
@github-actions
Copy link

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.

@82marbag 82marbag added this pull request to the merge queue Mar 23, 2023
@82marbag 82marbag merged commit d89a90d into main Mar 23, 2023
@82marbag 82marbag deleted the request-id-response-headers branch March 23, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlbarber hlbarber hlbarber approved these changes

@LukeMathWalker LukeMathWalker LukeMathWalker approved these changes

@drganjoo drganjoo Awaiting requested review from drganjoo drganjoo was automatically assigned from awslabs/smithy-rs-server

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@82marbag @LukeMathWalker @hlbarber