breaking: Updated dpp::role comparison operators to now consider role IDs (#1333)

Author: Commandserver

docpages/example_programs/misc/checking-member-permissions.md

@@ -15,6 +15,34 @@ if (c && c->get_user_permissions(member).can(dpp::p_send_messages)) {
 }
 ```
 
+### Role Hierarchy
+
+The recommended and correct way to compare for roles in the hierarchy is using the comparison operators (`<`, `>`) on the \ref dpp::role::operator<(dpp::role, dpp::role) "dpp::role" objects themselves.
+Keep in mind that multiple roles can have the same position number.
+As a result, comparing roles by position alone can lead to subtle bugs when checking for role hierarchy.
+
+For example let's say you have a ban command, and want to make sure that any issuer of the command can only ban members lower position than their own highest role.
+
+```cpp
+bot.on_interaction_create([](const dpp::interaction_create_t& event) {
+    dpp::snowflake target_id = std::get<dpp::snowflake>(event.get_parameter("user"));
+    dpp::guild_member target = event.command.get_resolved_member(target_id);
+
+    for (dpp::snowflake issuer_role_id : event.command.member.get_roles()) {
+        auto issuer_role = dpp::find_role(issuer_role_id);
+        if (issuer_role == nullptr) continue;
+        for (dpp::snowflake target_role_id : target.get_roles()) {
+            auto target_role = dpp::find_role(target_role_id);
+            if (target_role == nullptr) continue;
+            if (target_role > issuer_role) {
+                event.reply("You can't ban someone whose role is higher than yours!");
+                return;
+            }
+        }
+    }
+});
+```
+
 ## Permissions in Interaction Events
 
 ### Default Command Permissions

include/dpp/cluster.h

@@ -2170,8 +2170,8 @@ class DPP_EXPORT cluster {
 	 * @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
 	 * @param c Channel to set permissions for
 	 * @param overwrite_id Overwrite to change (a user or role ID)
-	 * @param allow allow permissions bitmask
-	 * @param deny deny permissions bitmask
+	 * @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
+	 * @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
 	 * @param member true if the overwrite_id is a user id, false if it is a channel id
 	 * @param callback Function to call when the API call completes.
 	 * On success the callback will contain a dpp::confirmation object in confirmation_callback_t::value. On failure, the value is undefined and confirmation_callback_t::is_error() method will return true. You can obtain full error details with confirmation_callback_t::get_error().
@@ -2185,8 +2185,8 @@ class DPP_EXPORT cluster {
 	 * @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
 	 * @param channel_id ID of the channel to set permissions for
 	 * @param overwrite_id Overwrite to change (a user or role ID)
-	 * @param allow allow permissions bitmask
-	 * @param deny deny permissions bitmask
+	 * @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
+	 * @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
 	 * @param member true if the overwrite_id is a user id, false if it is a channel id
 	 * @param callback Function to call when the API call completes.
 	 * On success the callback will contain a dpp::confirmation object in confirmation_callback_t::value. On failure, the value is undefined and confirmation_callback_t::is_error() method will return true. You can obtain full error details with confirmation_callback_t::get_error().

include/dpp/cluster_sync_calls.h

@@ -590,8 +590,8 @@ DPP_DEPRECATED("Please use coroutines instead of sync functions: https://dpp.dev
  * @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
  * @param c Channel to set permissions for
  * @param overwrite_id Overwrite to change (a user or role ID)
- * @param allow allow permissions bitmask
- * @param deny deny permissions bitmask
+ * @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
+ * @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
  * @param member true if the overwrite_id is a user id, false if it is a channel id
  * @return confirmation returned object on completion
  * \memberof dpp::cluster
@@ -610,8 +610,8 @@ DPP_DEPRECATED("Please use coroutines instead of sync functions: https://dpp.dev
  * @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
  * @param channel_id ID of the channel to set permissions for
  * @param overwrite_id Overwrite to change (a user or role ID)
- * @param allow allow permissions bitmask
- * @param deny deny permissions bitmask
+ * @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
+ * @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
  * @param member true if the overwrite_id is a user id, false if it is a channel id
  * @return confirmation returned object on completion
  * \memberof dpp::cluster

include/dpp/role.h

@@ -120,6 +120,10 @@ class DPP_EXPORT role : public managed, public json_interface<role> {
 
 	/**
 	 * @brief Role position.
+	 *
+	 * @warning multiple roles can have the same position number.
+	 * As a result, comparing roles by position alone can lead to subtle bugs when checking for role hierarchy.
+	 * The recommended and correct way to compare for roles in the hierarchy is using the comparison operators on the role objects themselves.
 	 */
 	uint8_t position{0};
 
@@ -322,6 +326,11 @@ class DPP_EXPORT role : public managed, public json_interface<role> {
 	 * @return true if lhs is less than rhs
 	 */
 	friend inline bool operator< (const role& lhs, const role& rhs) {
+		if (lhs.position == rhs.position) {
+			// the higher the IDs are, the lower the position
+			return lhs.id > rhs.id;
+		}
+
 		return lhs.position < rhs.position;
 	}
 
@@ -333,7 +342,7 @@ class DPP_EXPORT role : public managed, public json_interface<role> {
 	 * @return true if lhs is greater than rhs
 	 */
 	friend inline bool operator> (const role& lhs, const role& rhs) {
-		return lhs.position > rhs.position;
+		return !(lhs < rhs);
 	}
 
 	/**
@@ -343,7 +352,7 @@ class DPP_EXPORT role : public managed, public json_interface<role> {
 	 * @return true if is equal to other
 	 */
 	inline bool operator== (const role& other) const {
-		return this->position == other.position;
+		return this->id == other.id;
 	}
 
 	/**
@@ -353,7 +362,7 @@ class DPP_EXPORT role : public managed, public json_interface<role> {
 	 * @return true if is not equal to other
 	 */
 	inline bool operator!= (const role& other) const {
-		return this->position != other.position;
+		return this->id != other.id;
 	}
 
 	/**

src/unittest/test.cpp

@@ -342,11 +342,28 @@ Markdown lol \\|\\|spoiler\\|\\| \\~\\~strikethrough\\~\\~ \\`small \\*code\\* b
 	set_test(TIMESTAMPTOSTRING, false);
 	set_test(TIMESTAMPTOSTRING, dpp::ts_to_string(1642611864) == "2022-01-19T17:04:24Z");
 
-	set_test(ROLE_COMPARE, false);
-	dpp::role role_1, role_2;
-	role_1.position = 1;
-	role_2.position = 2;
-	set_test(ROLE_COMPARE, role_1 < role_2 && role_1 != role_2);
+	{
+		set_test(ROLE_COMPARE, false);
+		dpp::role role_1, role_2;
+		role_1.id = 99;
+		role_1.position = 1;
+		role_1.guild_id = 123;
+		role_1.id = 98;
+		role_2.position = 2;
+		role_2.guild_id = 123;
+		set_test(ROLE_COMPARE, role_1 < role_2 && !(role_1 > role_2) && role_1 != role_2);
+	}
+	{
+		set_test(ROLE_COMPARE_CONSIDERING_ID, false);
+		dpp::role role_1, role_2;
+		role_1.id = 99;
+		role_1.position = 2;
+		role_1.guild_id = 123;
+		role_2.id = 98;
+		role_2.position = 2;
+		role_2.guild_id = 123;
+		set_test(ROLE_COMPARE_CONSIDERING_ID, role_1 < role_2 && !(role_1 > role_2));
+	}
 
 	set_test(WEBHOOK, false);
 	try {

src/unittest/test.h

@@ -206,6 +206,7 @@ DPP_TEST(UTILITY_CDN_ENDPOINT_URL_HASH, "utility::cdn_endpoint_url_hash", tf_off
 DPP_TEST(STICKER_GET_URL, "sticker::get_url aka utility::cdn_endpoint_url_sticker", tf_offline);
 DPP_TEST(EMOJI_GET_URL, "emoji::get_url", tf_offline);
 DPP_TEST(ROLE_COMPARE, "role::operator<", tf_offline);
+DPP_TEST(ROLE_COMPARE_CONSIDERING_ID, "role::operator<", tf_offline);
 DPP_TEST(ROLE_CREATE, "cluster::role_create", tf_online);
 DPP_TEST(ROLE_EDIT, "cluster::role_edit", tf_online);
 DPP_TEST(ROLE_DELETE, "cluster::role_delete", tf_online);