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

Provide a reason for not acceptable server request rejections #2607

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

david-perez
Copy link
Contributor

This commit makes it so that when an incoming request's Accept header
value set cannot be satisfied by the server, the unsatisfiable value set
is stored in the RequestRejection. The rejection reason will thus be
DEBUG-logged.


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

This commit makes it so that when an incoming request's `Accept` header
value set cannot be satisfied by the server, the unsatisfiable value set
is stored in the `RequestRejection`. The rejection reason will thus be
`DEBUG`-logged.
@david-perez david-perez added the server Rust server SDK label Apr 20, 2023
@david-perez david-perez requested a review from a team as a code owner April 20, 2023 12:30
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

}
// Must be of the form: type/subtype
// Must be of the form: `type/subtype`.
let content_type = content_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: I feel like this conversion can be done in a static once_cell::Lazy outside of this function, then we pass the &Mime into this function instead. That way we're not doing it over and over.

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 looked at Mime::parse and it appears to be a non-negligible process, so putting Mime behind a OnceCell and incurring an atomic read should indeed speed things up.

I also learnt that mime is not actively maintained, and stumbled upon mediatype, which is const-constructible, so for our static mime type coming from the sSDK I think it would make more sense to switch to that. It's also zero-copy, so parsing the request's Accept and Content-Type headers' mime types and performing the type + subtype equality check should be faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a plan

Copy link
Contributor

Choose a reason for hiding this comment

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

For the first part: #2629

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2629 is throwaway work if we switch away from mime. I've opened #2666 to track that.

@smithy-lang smithy-lang deleted a comment from crisidev Apr 21, 2023
// This is used across different protocol-specific `rejection` modules.
#[derive(Debug, Error)]
pub enum NotAcceptableReason {
#[error("cannot satisfy any of `Accept` header values: {0:?}")]
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't log values

@82marbag 82marbag mentioned this pull request Apr 25, 2023
2 tasks
82marbag added a commit that referenced this pull request Apr 26, 2023
## Description

Do not parse and initialize the mime, known at compile time, on every
request.

See
#2607 (comment)


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

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

---------

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
david-perez pushed a commit that referenced this pull request May 18, 2023
## Description

Do not parse and initialize the mime, known at compile time, on every
request.

See
#2607 (comment)


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

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

---------

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
david-perez pushed a commit that referenced this pull request May 22, 2023
## Description

Do not parse and initialize the mime, known at compile time, on every
request.

See
#2607 (comment)


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

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

---------

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
david-perez pushed a commit that referenced this pull request May 22, 2023
## Description

Do not parse and initialize the mime, known at compile time, on every
request.

See
#2607 (comment)


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

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

---------

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@rcoh rcoh requested a review from a team as a code owner November 14, 2023 02:21
@jmklix jmklix closed this Apr 12, 2024
@david-perez david-perez reopened this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants