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

Adds better abilities to check, what exactly was rate limited #2901

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

Conversation

TheCataliasTNT2k
Copy link

Add "OwnedRoute" to "RatelimitInfo" which allows to check the cause of the rate limit and which identifiers were affected (e.g. which channel).

@TheCataliasTNT2k TheCataliasTNT2k changed the base branch from current to next June 12, 2024 22:47
@github-actions github-actions bot added the http Related to the `http` module. label Jun 12, 2024
@TheCataliasTNT2k
Copy link
Author

The failing check seems broken, at least I do not get any error when looking at the details...

@jamesbt365
Copy link
Member

The failing check seems broken, at least I do not get any error when looking at the details...

cargo +nightly fmt --all

Comment on lines +36 to +39
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
enum RouteKind {
$($name,)+
}
Copy link
Member

Choose a reason for hiding this comment

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

Why was this moved?

Copy link
Author

Choose a reason for hiding this comment

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

To have two similar blocks next to another (Route and OwnedRoute).
I could move it back, but in my opinion it is more readable like this.

/// please match the variants you are interested in.
#[must_use]
#[allow(unused_variables)] // prevent compiler from complaining about unused variables
pub fn get_common_identifiers(&self) -> RatelimitCause {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn get_common_identifiers(&self) -> RatelimitCause {
pub fn cause(&self) -> RatelimitCause {

Copy link
Author

Choose a reason for hiding this comment

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

This does not really return the "cause" of the rate limit but the parts of the path, which was rate limited.
The "cause" would be the deletion of a message or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

So the type should not be caused RatelimitCause, it should be called RatelimitPathParts or something else.

Copy link
Author

Choose a reason for hiding this comment

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

That is better, yes.
Can the method name stay then?

@@ -27,23 +27,24 @@ enum RatelimitingKind {
macro_rules! routes {
($lt:lifetime, {
$(
$name:ident $({ $($field_name:ident: $field_type:ty),* })?,
$name:ident $({ $($field_name_route:ident: $field_type:ty | $field_type_owned:ty),* })?,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making the macro syntax messier, can you make this use traits instead? I'm pretty sure ToOwned should handle this, but if it doesn't you can just write a simple private trait.

Copy link
Author

Choose a reason for hiding this comment

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

How? I somehow need to know the type, which the OwnedRoute should use. I do not know, how a trait would be able to do that.
The problem is the "&str" type. Can you please provide an example for how to do it?

Copy link
Member

Choose a reason for hiding this comment

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

The owned type is simply <$field_type as ToOwned>::Owned, and if that doesn't return the correct types you can duplicate the definition of ToOwned and implement it for the types needed.

Copy link
Author

Choose a reason for hiding this comment

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

Will do that.

Copy link
Author

@TheCataliasTNT2k TheCataliasTNT2k Nov 22, 2024

Choose a reason for hiding this comment

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

The field_type_owned is actually needed, I did not find a way to get compiling code without it.

Reason:
The macro would expand into something like this (irrelevant code removed):

#[derive(Clone, Debug)]
pub enum OwnedRoute {
    ChannelMessageReaction { channel_id: <ChannelId as ToOwned>::Owned, message_id: <MessageId as ToOwned>::Owned, user_id: <UserId as ToOwned>::Owned, reaction: <&'a str as ToOwned>::Owned},
}

<&'a str as ToOwned>::Owned is not valid, because 'a does not exist. I could add the lifetime to the enum, but then we would need to carry this useless lifetime though the whole code. This can not be fixed by replacing the <&'a str as ToOwned>::Owned with a custom macro, I already tried:

macro_rules! get_owned_type {
    (&'a str) => {String};
    ($type:ty) => {<$type as ToOwned>::Owned};
}

#[derive(Clone, Debug)]
pub enum OwnedRoute {
    ChannelMessageReaction { channel_id: get_owned_type!(ChannelId), message_id: get_owned_type!(MessageId), user_id: get_owned_type!(UserId), reaction: get_owned_type!(&'a str)},
}

will not compile for the same reason.

The compiler is not smart enough to ignore the unused lifetime at the moment.

}

#[must_use]
pub fn get_owned_route(&self) -> OwnedRoute {
Copy link
Member

Choose a reason for hiding this comment

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

Rust convention is to name getters without the prefix, but since this is a &self -> Owned I think the convention would be to_owned_route... however that's just ToOwned, so implement ToOwned instead.

Copy link
Author

Choose a reason for hiding this comment

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

I did not use toOwned, because this is not really a clone and should not be treated as one, which is expected by the trait however.

Copy link
Member

Choose a reason for hiding this comment

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

With that logic ToOwned should not be a thing, but this is exactly what it's for. This is internally just converting str -> String which is exactly the point of ToOwned.

Copy link
Author

@TheCataliasTNT2k TheCataliasTNT2k Nov 22, 2024

Choose a reason for hiding this comment

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

This can not be done using ToOwned, because this would require the "copied" object to carry the lifetime with it.

I already tried that sometime, and it makes the whole code a mess.

I renamed the method though (not pushed yet because of the other change requests).

Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

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

I cannot review this PR without it being rebased correctly on-top of next.

@TheCataliasTNT2k TheCataliasTNT2k force-pushed the better_ratelimits branch 2 times, most recently from cf3701f to ec3a62e Compare November 25, 2024 00:17
@TheCataliasTNT2k
Copy link
Author

I cannot review this PR without it being rebased correctly on-top of next.

Was doing that right as you wrote that, lol xD

It is just a copy of Route without any lifetimes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Related to the `http` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants