From 4dcbba199e69393a8b5d5dfa048c17379609fedf Mon Sep 17 00:00:00 2001 From: LasterAlex Date: Sun, 17 Nov 2024 01:21:00 +0200 Subject: [PATCH] Better errors + dptree integration --- crates/teloxide-macros/src/button_enum.rs | 12 +- .../teloxide-macros/src/fields_stringify.rs | 8 +- crates/teloxide-macros/src/inline_buttons.rs | 2 +- .../teloxide/src/dispatching/handler_ext.rs | 253 +++++++++++++----- crates/teloxide/src/utils/button.rs | 91 ++++++- crates/teloxide/src/utils/command.rs | 9 + crates/teloxide/tests/buttons.rs | 46 ++++ crates/teloxide/tests/callback_data.rs | 25 +- 8 files changed, 359 insertions(+), 87 deletions(-) create mode 100644 crates/teloxide/tests/buttons.rs diff --git a/crates/teloxide-macros/src/button_enum.rs b/crates/teloxide-macros/src/button_enum.rs index 8ebd7338..d2ac2215 100644 --- a/crates/teloxide-macros/src/button_enum.rs +++ b/crates/teloxide-macros/src/button_enum.rs @@ -17,9 +17,15 @@ impl ButtonEnum { variants_only_attr![rename]; - let separator = fields_separator - .map(|(s, _)| s) - .unwrap_or_else(|| String::from(DEFAULT_CALLBACK_DATA_SEPARATOR)); + let separator = match fields_separator { + Some((separator, sp)) => { + if separator.is_empty() { + compile_error_at("Separator can't be empty!", sp); + } + separator + } + None => String::from(DEFAULT_CALLBACK_DATA_SEPARATOR), + }; // We can just always use a separator parser, since the user won't ever interact // with that diff --git a/crates/teloxide-macros/src/fields_stringify.rs b/crates/teloxide-macros/src/fields_stringify.rs index 0c665408..02f480ea 100644 --- a/crates/teloxide-macros/src/fields_stringify.rs +++ b/crates/teloxide-macros/src/fields_stringify.rs @@ -36,7 +36,9 @@ pub(crate) fn impl_stringify_args_unnamed( if stringified.contains(fields_separator) { return ::std::result::Result::Err(StringifyError::SeparatorInUnnamedArgument { enum_variant: std::concat!(#self_string_name, "::", #string_variant).to_owned(), - field: #i + stringified_data: stringified, + separator: fields_separator.to_owned(), + field: #i, }); } stringified @@ -66,7 +68,9 @@ pub(crate) fn impl_stringify_args_named( if stringified.contains(fields_separator) { return ::std::result::Result::Err(StringifyError::SeparatorInNamedArgument { enum_variant: ::std::concat!(#self_string_name, "::", #string_variant).to_owned(), - argument: ::std::stringify!(#name).to_string() + stringified_data: stringified, + separator: fields_separator.to_owned(), + argument: ::std::stringify!(#name).to_string(), }); } stringified diff --git a/crates/teloxide-macros/src/inline_buttons.rs b/crates/teloxide-macros/src/inline_buttons.rs index b999dd34..a21d38a5 100644 --- a/crates/teloxide-macros/src/inline_buttons.rs +++ b/crates/teloxide-macros/src/inline_buttons.rs @@ -84,7 +84,7 @@ fn impl_parse( #( #matching_values => Ok(#variants_initialization), )* - _ => ::std::result::Result::Err(ParseError::UnknownCommand(enum_variant.to_owned())), + _ => ::std::result::Result::Err(ParseError::UnknownCallbackDataVariant(enum_variant.to_owned())), } } } diff --git a/crates/teloxide/src/dispatching/handler_ext.rs b/crates/teloxide/src/dispatching/handler_ext.rs index d45c29f1..84c1c08b 100644 --- a/crates/teloxide/src/dispatching/handler_ext.rs +++ b/crates/teloxide/src/dispatching/handler_ext.rs @@ -4,9 +4,10 @@ use crate::{ DpHandlerDescription, }, types::{Me, Message}, - utils::command::BotCommands, + utils::{button::InlineButtons, command::BotCommands}, }; use dptree::{di::DependencyMap, Handler}; +use teloxide_core::types::CallbackQuery; use std::fmt::Debug; @@ -35,6 +36,16 @@ pub trait HandlerExt { where C: BotCommands + Send + Sync + 'static; + /// Returns a handler that accepts a parsed callback query data `D`. + /// + /// ## Dependency requirements + /// + /// - [`crate::types::CallbackQuery`] + #[must_use] + fn filter_callback_data(self) -> Self + where + D: InlineButtons + Send + Sync + 'static; + /// Passes [`Dialogue`] and `D` as handler dependencies. /// /// It does so by the following steps: @@ -80,6 +91,13 @@ where self.chain(filter_mention_command::()) } + fn filter_callback_data(self) -> Self + where + D: InlineButtons + Send + Sync + 'static, + { + self.chain(filter_callback_data::()) + } + fn enter_dialogue(self) -> Self where S: Storage + ?Sized + Send + Sync + 'static, @@ -113,6 +131,28 @@ where }) } +/// Returns a handler that accepts a parsed callback query data `D` +/// +/// A call to this function is the same as +/// `dptree::entry().filter_callback_data()`. +/// +/// See [`HandlerExt::filter_callback_data`]. +/// +/// ## Dependency requirements +/// +/// - [`crate::types::CallbackQuery`] +#[must_use] +pub fn filter_callback_data( +) -> Handler<'static, DependencyMap, Output, DpHandlerDescription> +where + D: InlineButtons + Send + Sync + 'static, + Output: Send + Sync + 'static, +{ + dptree::filter_map(move |callback_query: CallbackQuery| { + callback_query.data.and_then(|data| D::parse(data.as_ref()).ok()) + }) +} + /// Returns a handler that accepts a parsed command `C` if the command /// contains a bot mention, for example `/start@my_bot`. /// @@ -151,13 +191,17 @@ where #[cfg(test)] #[cfg(feature = "macros")] mod tests { - use crate::{self as teloxide, dispatching::UpdateFilterExt, utils::command::BotCommands}; + use crate::{ + self as teloxide, + dispatching::UpdateFilterExt, + utils::{button::InlineButtons, command::BotCommands}, + }; use chrono::DateTime; use dptree::deps; use teloxide_core::types::{ - Chat, ChatFullInfo, ChatId, ChatKind, ChatPrivate, LinkPreviewOptions, Me, MediaKind, - MediaText, Message, MessageCommon, MessageId, MessageKind, Update, UpdateId, UpdateKind, - User, UserId, + CallbackQuery, Chat, ChatFullInfo, ChatId, ChatKind, ChatPrivate, LinkPreviewOptions, Me, + MediaKind, MediaText, Message, MessageCommon, MessageId, MessageKind, Update, UpdateId, + UpdateKind, User, UserId, }; use super::HandlerExt; @@ -168,78 +212,111 @@ mod tests { Test, } - fn make_update(text: String) -> Update { + #[derive(InlineButtons, Debug, PartialEq)] + enum CallbackButtons { + Button1, + Button2(String), + Button3 { field1: u32 }, + } + + fn make_from() -> User { + User { + id: UserId(109_998_024), + is_bot: false, + first_name: String::from("Laster"), + last_name: None, + username: Some(String::from("laster_alex")), + language_code: Some(String::from("en")), + is_premium: false, + added_to_attachment_menu: false, + } + } + + fn make_chat() -> Chat { + Chat { + id: ChatId(109_998_024), + kind: ChatKind::Private(ChatPrivate { + username: Some(String::from("Laster")), + first_name: Some(String::from("laster_alex")), + last_name: None, + bio: None, + has_private_forwards: None, + has_restricted_voice_and_video_messages: None, + business_intro: None, + business_location: None, + business_opening_hours: None, + birthdate: None, + personal_chat: None, + }), + photo: None, + available_reactions: None, + pinned_message: None, + message_auto_delete_time: None, + has_hidden_members: false, + has_aggressive_anti_spam_enabled: false, + chat_full_info: ChatFullInfo::default(), + } + } + + fn make_message(text: String) -> Message { let timestamp = 1_569_518_829; let date = DateTime::from_timestamp(timestamp, 0).unwrap(); + Message { + via_bot: None, + id: MessageId(5042), + thread_id: None, + from: Some(make_from()), + sender_chat: None, + is_topic_message: false, + sender_business_bot: None, + date, + chat: make_chat(), + kind: MessageKind::Common(MessageCommon { + reply_to_message: None, + forward_origin: None, + external_reply: None, + quote: None, + edit_date: None, + media_kind: MediaKind::Text(MediaText { + text, + entities: vec![], + link_preview_options: Some(LinkPreviewOptions { + is_disabled: true, + url: None, + prefer_small_media: false, + prefer_large_media: false, + show_above_text: false, + }), + }), + reply_markup: None, + author_signature: None, + is_automatic_forward: false, + has_protected_content: false, + reply_to_story: None, + sender_boost_count: None, + is_from_offline: false, + business_connection_id: None, + }), + } + } + + fn make_update(text: String) -> Update { + Update { id: UpdateId(326_170_274), kind: UpdateKind::Message(make_message(text)) } + } + + fn make_callback_query_update(data: String) -> Update { Update { - id: UpdateId(326_170_274), - kind: UpdateKind::Message(Message { - via_bot: None, - id: MessageId(5042), - thread_id: None, - from: Some(User { - id: UserId(109_998_024), - is_bot: false, - first_name: String::from("Laster"), - last_name: None, - username: Some(String::from("laster_alex")), - language_code: Some(String::from("en")), - is_premium: false, - added_to_attachment_menu: false, - }), - sender_chat: None, - is_topic_message: false, - sender_business_bot: None, - date, - chat: Chat { - id: ChatId(109_998_024), - kind: ChatKind::Private(ChatPrivate { - username: Some(String::from("Laster")), - first_name: Some(String::from("laster_alex")), - last_name: None, - bio: None, - has_private_forwards: None, - has_restricted_voice_and_video_messages: None, - business_intro: None, - business_location: None, - business_opening_hours: None, - birthdate: None, - personal_chat: None, - }), - photo: None, - available_reactions: None, - pinned_message: None, - message_auto_delete_time: None, - has_hidden_members: false, - has_aggressive_anti_spam_enabled: false, - chat_full_info: ChatFullInfo::default(), - }, - kind: MessageKind::Common(MessageCommon { - reply_to_message: None, - forward_origin: None, - external_reply: None, - quote: None, - edit_date: None, - media_kind: MediaKind::Text(MediaText { - text, - entities: vec![], - link_preview_options: Some(LinkPreviewOptions { - is_disabled: true, - url: None, - prefer_small_media: false, - prefer_large_media: false, - show_above_text: false, - }), - }), - reply_markup: None, - author_signature: None, - is_automatic_forward: false, - has_protected_content: false, - reply_to_story: None, - sender_boost_count: None, - is_from_offline: false, - business_connection_id: None, - }), + id: UpdateId(326_170_275), + kind: UpdateKind::CallbackQuery(CallbackQuery { + id: "5024".to_string(), + from: make_from(), + message: Some(teloxide_core::types::MaybeInaccessibleMessage::Regular( + make_message("text".to_owned()), + )), + inline_message_id: None, + chat_instance: "12345678".to_owned(), + data: Some(data), + game_short_name: None, }), } } @@ -300,4 +377,32 @@ mod tests { let result = h.dispatch(deps![update, me.clone()]).await; assert!(result.is_continue()); } + + #[tokio::test] + async fn test_filter_callback_data() { + let h = dptree::entry().branch( + Update::filter_callback_query() + .filter_callback_data::() + .endpoint(|| async {}), + ); + + let button = CallbackButtons::Button1; + let update = make_callback_query_update(button.stringify().unwrap()); + let result = h.dispatch(deps![update]).await; + assert!(result.is_break()); + + let button = CallbackButtons::Button2("SomeData".to_owned()); + let update = make_callback_query_update(button.stringify().unwrap()); + let result = h.dispatch(deps![update]).await; + assert!(result.is_break()); + + let button = CallbackButtons::Button3 { field1: 123 }; + let update = make_callback_query_update(button.stringify().unwrap()); + let result = h.dispatch(deps![update]).await; + assert!(result.is_break()); + + let update = make_callback_query_update("wrong_data".to_string()); + let result = h.dispatch(deps![update]).await; + assert!(result.is_continue()); + } } diff --git a/crates/teloxide/src/utils/button.rs b/crates/teloxide/src/utils/button.rs index 844c2959..be576471 100644 --- a/crates/teloxide/src/utils/button.rs +++ b/crates/teloxide/src/utils/button.rs @@ -1,5 +1,8 @@ //! Docs later +use std::fmt::{Display, Formatter}; + use super::command::ParseError; +use teloxide_core::types::InlineKeyboardButton; #[cfg(feature = "macros")] pub use teloxide_macros::InlineButtons; @@ -10,6 +13,17 @@ pub trait InlineButtons: Sized { /// Stringifies the callback data. fn stringify(self) -> Result; + + /// Builds an [`InlineKeyboardButton`] from the enum variant + /// + /// [`InlineKeyboardButton`]: crate::types::InlineKeyboardButton + fn build_button(self, text: T) -> Result + where + T: Into, + { + let callback_data = self.stringify()?; + Ok(InlineKeyboardButton::callback(text.into(), callback_data)) + } } /// Errors returned from [`InlineButtons::stringify`]. @@ -17,6 +31,79 @@ pub trait InlineButtons: Sized { /// [`InlineButtons::stringify`]: InlineButtons::stringify #[derive(Debug)] pub enum StringifyError { - SeparatorInNamedArgument { enum_variant: String, argument: String }, - SeparatorInUnnamedArgument { enum_variant: String, field: usize }, + SeparatorInNamedArgument { + enum_variant: String, + stringified_data: String, + separator: String, + argument: String, + }, + SeparatorInUnnamedArgument { + enum_variant: String, + stringified_data: String, + separator: String, + field: usize, + }, } + +fn make_pointers_string(prefix: String, text: String, point_to: String) -> String { + let all_indexes: Vec<_> = text.match_indices(&point_to).collect(); + let prefix_len = prefix.chars().count(); + let mut pointers_vec = vec![" "; prefix_len + text.chars().count()]; + + for (start, matched) in &all_indexes { + for (i, _) in matched.chars().enumerate() { + pointers_vec[prefix_len + start + i] = "^"; + } + } + pointers_vec.join("") +} + +impl Display for StringifyError { + fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> { + match self { + Self::SeparatorInNamedArgument { + enum_variant, + stringified_data, + separator, + argument, + } => { + let prefix = format!("self.{argument} == \""); + // Makes ^^^ pointers to where the separator is + let separator_pointers_string = make_pointers_string( + prefix.clone(), + stringified_data.to_string(), + separator.to_string(), + ); + write!( + f, + "There is a separator \"{separator}\" in `{enum_variant}`: \ + \n{prefix}{stringified_data}\"\n{separator_pointers_string}\n\nPlease \ + consider changing the separator with \ + `#[button(fields_separator=\"NEW_SEPARATOR\")]`" + ) + } + Self::SeparatorInUnnamedArgument { + enum_variant, + stringified_data, + separator, + field, + } => { + let prefix = format!("self.{field} == \""); + let separator_pointers_string = make_pointers_string( + prefix.clone(), + stringified_data.to_string(), + separator.to_string(), + ); + write!( + f, + "There is a separator \"{separator}\" in `{enum_variant}`: \ + \n{prefix}{stringified_data}\"\n{separator_pointers_string}\n\nPlease \ + consider changing the separator with \ + `#[button(fields_separator=\"NEW_SEPARATOR\")]`" + ) + } + } + } +} + +impl std::error::Error for StringifyError {} diff --git a/crates/teloxide/src/utils/command.rs b/crates/teloxide/src/utils/command.rs index 0dce838a..7a46e032 100644 --- a/crates/teloxide/src/utils/command.rs +++ b/crates/teloxide/src/utils/command.rs @@ -266,6 +266,7 @@ pub trait BotCommands: Sized { pub type PrefixedBotCommand = String; pub type BotName = String; +pub type CallbackDataVariant = String; /// Errors returned from [`BotCommands::parse`]. /// @@ -291,6 +292,11 @@ pub enum ParseError { UnknownCommand(PrefixedBotCommand), WrongBotName(BotName), + /// Error for [`InlineButtons`]. + /// + /// [`InlineButtons`]: crate::utils::button::InlineButtons + UnknownCallbackDataVariant(CallbackDataVariant), + /// A custom error which you can return from your custom parser. Custom(Box), } @@ -472,6 +478,9 @@ impl Display for ParseError { ), ParseError::IncorrectFormat(e) => write!(f, "Incorrect format of command args: {e}"), ParseError::UnknownCommand(e) => write!(f, "Unknown command: {e}"), + ParseError::UnknownCallbackDataVariant(v) => { + write!(f, "Unknown callback data variant: {v}") + } ParseError::WrongBotName(n) => write!(f, "Wrong bot name: {n}"), ParseError::Custom(e) => write!(f, "{e}"), } diff --git a/crates/teloxide/tests/buttons.rs b/crates/teloxide/tests/buttons.rs new file mode 100644 index 00000000..e0def5a0 --- /dev/null +++ b/crates/teloxide/tests/buttons.rs @@ -0,0 +1,46 @@ +#[cfg(feature = "macros")] +use teloxide::utils::button::InlineButtons; + +// We put tests here because macro expand in unit tests in module +// teloxide::utils::button was a failure + +#[cfg(feature = "macros")] +#[derive(InlineButtons, Debug, PartialEq)] +enum CallbackButtons { + Button1, + Button2(String), + Button3 { field1: u32 }, +} + +#[test] +#[cfg(feature = "macros")] +fn test_make_button() { + use teloxide::types::InlineKeyboardButton; + + let text = "Text for button 1"; + let actual = CallbackButtons::Button1.build_button(text).unwrap(); + let expected = InlineKeyboardButton::callback(text, "Button1"); + assert_eq!(actual, expected); +} + +#[test] +#[cfg(feature = "macros")] +fn test_make_button_with_unnamed_args() { + use teloxide::types::InlineKeyboardButton; + + let text = "Text for button 2"; + let actual = CallbackButtons::Button2("data".to_owned()).build_button(text).unwrap(); + let expected = InlineKeyboardButton::callback(text, "Button2;data"); + assert_eq!(actual, expected); +} + +#[test] +#[cfg(feature = "macros")] +fn test_make_button_with_named_args() { + use teloxide::types::InlineKeyboardButton; + + let text = "Text for button 3"; + let actual = CallbackButtons::Button3 { field1: 23 }.build_button(text).unwrap(); + let expected = InlineKeyboardButton::callback(text, "Button3;23"); + assert_eq!(actual, expected); +} diff --git a/crates/teloxide/tests/callback_data.rs b/crates/teloxide/tests/callback_data.rs index be3ab75e..d0ae3035 100644 --- a/crates/teloxide/tests/callback_data.rs +++ b/crates/teloxide/tests/callback_data.rs @@ -2,7 +2,7 @@ use teloxide::utils::button::InlineButtons; // We put tests here because macro expand in unit tests in module -// teloxide::utils::command was a failure +// teloxide::utils::button was a failure #[test] #[cfg(feature = "macros")] @@ -49,10 +49,18 @@ fn stringify_button_error() { } let button = DefaultData::Fruit("test;test2".to_string()); + match button.stringify() { - Err(StringifyError::SeparatorInUnnamedArgument { enum_variant, field }) => { + Err(StringifyError::SeparatorInUnnamedArgument { + enum_variant, + stringified_data, + separator, + field, + }) => { assert_eq!(field, 0); - assert_eq!(enum_variant, "DefaultData::Fruit") + assert_eq!(enum_variant, "DefaultData::Fruit"); + assert_eq!(stringified_data, "test;test2"); + assert_eq!(separator, ";"); } _ => panic!("Expected an error!"), } @@ -121,9 +129,16 @@ fn stringify_button_named_fields_error() { let button = DefaultData::Fruit { num: 9, data: "test;test2".to_owned() }; match button.stringify() { - Err(StringifyError::SeparatorInNamedArgument { enum_variant, argument }) => { + Err(StringifyError::SeparatorInNamedArgument { + enum_variant, + stringified_data, + separator, + argument, + }) => { assert_eq!(argument, "data".to_owned()); - assert_eq!(enum_variant, "DefaultData::Fruit") + assert_eq!(enum_variant, "DefaultData::Fruit"); + assert_eq!(stringified_data, "test;test2"); + assert_eq!(separator, ";"); } _ => panic!("Expected an error!"), }