diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp index 36ab75675..100a4ebc6 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp @@ -49,6 +49,7 @@ #include #include #include +#include #include Server_AbstractPlayer::Server_AbstractPlayer(Server_Game *_game, @@ -1091,8 +1092,9 @@ Server_AbstractPlayer::cmdCreateToken(const Command_CreateToken &cmd, ResponseCo _event.set_zone_name(card->getZone()->getName().toStdString()); _event.set_card_id(card->getId()); - card->setCounter(i.key(), i.value(), &_event); - ges.enqueueGameEvent(_event, playerId); + if (card->setCounter(i.key(), i.value(), &_event)) { + ges.enqueueGameEvent(_event, playerId); + } } // Copy parent card @@ -1338,8 +1340,9 @@ Response::ResponseCode Server_AbstractPlayer::cmdSetCardCounter(const Command_Se Event_SetCardCounter event; event.set_zone_name(zone->getName().toStdString()); event.set_card_id(card->getId()); - card->setCounter(cmd.counter_id(), cmd.counter_value(), &event); - ges.enqueueGameEvent(event, playerId); + if (card->setCounter(cmd.counter_id(), cmd.counter_value(), &event)) { + ges.enqueueGameEvent(event, playerId); + } return Response::RespOk; } @@ -1368,14 +1371,13 @@ Response::ResponseCode Server_AbstractPlayer::cmdIncCardCounter(const Command_In return Response::RespNameNotFound; } - int newValue = card->getCounter(cmd.counter_id()) + cmd.counter_delta(); - card->setCounter(cmd.counter_id(), newValue); - Event_SetCardCounter event; event.set_zone_name(zone->getName().toStdString()); event.set_card_id(card->getId()); - event.set_counter_id(cmd.counter_id()); - event.set_counter_value(newValue); + if (!card->incrementCounter(cmd.counter_id(), cmd.counter_delta(), &event)) { + return Response::RespOk; + } + ges.enqueueGameEvent(event, playerId); return Response::RespOk; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp index e3d53ace8..3532abb0b 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp @@ -26,6 +26,7 @@ #include #include #include +#include Server_Card::Server_Card(const CardRef &cardRef, int _id, int _coord_x, int _coord_y, Server_CardZone *_zone) : zone(_zone), id(_id), coord_x(_coord_x), coord_y(_coord_y), cardRef(cardRef), tapped(false), attacking(false), @@ -110,8 +111,13 @@ QString Server_Card::setAttribute(CardAttribute attribute, const QString &avalue return avalue; } -void Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) +bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) { + const int oldValue = counters.value(_id, 0); + if (value == oldValue) { + return false; + } + if (value) { counters.insert(_id, value); } else { @@ -122,6 +128,35 @@ void Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) event->set_counter_id(_id); event->set_counter_value(value); } + + return true; +} + +bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event) +{ + // TODO: Extract overflow-safe arithmetic into shared helper. + // Duplicated in Server_Counter::incrementCount() - keep in sync if modified. + const int oldValue = counters.value(counterId, 0); + const auto result = static_cast(oldValue) + static_cast(delta); + const int newValue = static_cast(qBound(static_cast(std::numeric_limits::min()), result, + static_cast(std::numeric_limits::max()))); + + if (newValue == oldValue) { + return false; + } + + if (newValue) { + counters.insert(counterId, newValue); + } else { + counters.remove(counterId); + } + + if (event) { + event->set_counter_id(counterId); + event->set_counter_value(newValue); + } + + return true; } void Server_Card::setParentCard(Server_Card *_parentCard) diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h index bc326bbc4..41304ef99 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h @@ -153,7 +153,24 @@ public: { cardRef = _cardRef; } - void setCounter(int _id, int value, Event_SetCardCounter *event = nullptr); + /** + * @brief Sets a card counter to an exact value. + * @param _id The counter ID. + * @param value The new value (0 removes the counter). + * @param event Optional event to populate with counter state. + * @return true if the value changed, false otherwise. + */ + [[nodiscard]] bool setCounter(int _id, int value, Event_SetCardCounter *event = nullptr); + /** + * @brief Increments a card counter with overflow-safe arithmetic. + * @param counterId The counter ID to modify. + * @param delta The amount to add (may be negative for decrement). + * @param event Optional event to populate with counter state. + * @return true if the value changed, false otherwise. + * @note If counter does not exist, starts from 0. Counter is removed if result is 0. + * @warning For direct assignment without clamping, use setCounter(). + */ + [[nodiscard]] bool incrementCounter(int counterId, int delta, Event_SetCardCounter *event = nullptr); void setTapped(bool _tapped) { tapped = _tapped; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h index 55aad991c..3bd8d380c 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h @@ -22,9 +22,22 @@ #include #include +#include class ServerInfo_Counter; +/** + * @class Server_Counter + * @brief Represents a player counter with overflow-safe increment arithmetic. + * + * All value modifications return whether the value actually changed, + * enabling callers to skip unnecessary network events. + * + * @note Direct assignment via setCount() does not clamp; only + * incrementCount() enforces int boundary saturation. + * @note Unlike card counters, player counters are never auto-removed + * when they reach zero - they persist with value 0. + */ class Server_Counter { protected: @@ -59,11 +72,42 @@ public: { return count; } - void setCount(int _count) + + /** + * @brief Sets the counter to an exact value. + * @param _count The new value (assigned directly without clamping). + * @return true if the value changed, false otherwise. + * @warning This performs raw assignment. For overflow-safe incrementing, + * use incrementCount(). + */ + [[nodiscard]] bool setCount(int _count) { + const int oldCount = count; count = _count; + return count != oldCount; } + /** + * @brief Increments the counter by delta with overflow-safe arithmetic. + * @param delta The amount to add (may be negative for decrement). + * @return true if the value changed, false otherwise. + * @note Clamps result to [INT_MIN, INT_MAX] to prevent overflow. + */ + [[nodiscard]] bool incrementCount(int delta) + { + // TODO: Extract overflow-safe arithmetic into shared helper. + // Duplicated in Server_Card::incrementCounter() - keep in sync if modified. + const int oldCount = count; + const auto result = static_cast(count) + static_cast(delta); + count = static_cast(qBound(static_cast(std::numeric_limits::min()), result, + static_cast(std::numeric_limits::max()))); + return count != oldCount; + } + + /** + * @brief Populates info with this counter's current state for network serialization. + * @param info The protobuf message to populate. + */ void getInfo(ServerInfo_Counter *info); }; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp index a1a0a3b3a..56e3f9f8e 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp @@ -436,17 +436,19 @@ Server_Player::cmdIncCounter(const Command_IncCounter &cmd, ResponseContainer & return Response::RespContextError; } - Server_Counter *c = counters.value(cmd.counter_id(), 0); + const int counterId = cmd.counter_id(); + Server_Counter *c = counters.value(counterId, nullptr); if (!c) { return Response::RespNameNotFound; } - c->setCount(c->getCount() + cmd.delta()); - - Event_SetCounter event; - event.set_counter_id(c->getId()); - event.set_value(c->getCount()); - ges.enqueueGameEvent(event, playerId); + bool didChange = c->incrementCount(cmd.delta()); + if (didChange) { + Event_SetCounter event; + event.set_counter_id(c->getId()); + event.set_value(c->getCount()); + ges.enqueueGameEvent(event, playerId); + } return Response::RespOk; } @@ -487,17 +489,19 @@ Server_Player::cmdSetCounter(const Command_SetCounter &cmd, ResponseContainer & return Response::RespContextError; } - Server_Counter *c = counters.value(cmd.counter_id(), 0); + const int counterId = cmd.counter_id(); + Server_Counter *c = counters.value(counterId, nullptr); if (!c) { return Response::RespNameNotFound; } - c->setCount(cmd.value()); - - Event_SetCounter event; - event.set_counter_id(c->getId()); - event.set_value(c->getCount()); - ges.enqueueGameEvent(event, playerId); + bool didChange = c->setCount(cmd.value()); + if (didChange) { + Event_SetCounter event; + event.set_counter_id(c->getId()); + event.set_value(c->getCount()); + ges.enqueueGameEvent(event, playerId); + } return Response::RespOk; } @@ -512,15 +516,16 @@ Server_Player::cmdDelCounter(const Command_DelCounter &cmd, ResponseContainer & return Response::RespContextError; } - Server_Counter *counter = counters.value(cmd.counter_id(), 0); + const int counterId = cmd.counter_id(); + Server_Counter *counter = counters.value(counterId, nullptr); if (!counter) { return Response::RespNameNotFound; } - counters.remove(cmd.counter_id()); + counters.remove(counterId); delete counter; Event_DelCounter event; - event.set_counter_id(cmd.counter_id()); + event.set_counter_id(counterId); ges.enqueueGameEvent(event, playerId); return Response::RespOk; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index fffaf1bda..00eba288e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -6,6 +6,8 @@ add_test(NAME dummy_test COMMAND dummy_test) add_test(NAME expression_test COMMAND expression_test) add_test(NAME test_age_formatting COMMAND test_age_formatting) add_test(NAME password_hash_test COMMAND password_hash_test) +add_test(NAME server_card_counter_test COMMAND server_card_counter_test) +add_test(NAME server_counter_test COMMAND server_counter_test) add_test(NAME deck_hash_performance_test COMMAND deck_hash_performance_test) set_tests_properties(deck_hash_performance_test PROPERTIES TIMEOUT 5) @@ -17,6 +19,8 @@ add_executable(expression_test expression_test.cpp) add_executable(test_age_formatting test_age_formatting.cpp) add_executable(password_hash_test password_hash_test.cpp) add_executable(deck_hash_performance_test deck_hash_performance_test.cpp) +add_executable(server_card_counter_test server_card_counter_test.cpp) +add_executable(server_counter_test server_counter_test.cpp) find_package(GTest) @@ -48,6 +52,8 @@ if(NOT GTEST_FOUND) add_dependencies(test_age_formatting gtest) add_dependencies(password_hash_test gtest) add_dependencies(deck_hash_performance_test gtest) + add_dependencies(server_card_counter_test gtest) + add_dependencies(server_counter_test gtest) endif() include_directories(${GTEST_INCLUDE_DIRS}) @@ -61,6 +67,12 @@ target_link_libraries( deck_hash_performance_test libcockatrice_deck_list libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) +target_link_libraries( + server_card_counter_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) +target_link_libraries( + server_counter_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) add_subdirectory(card_zone_algorithms) add_subdirectory(carddatabase) diff --git a/tests/server_card_counter_test.cpp b/tests/server_card_counter_test.cpp new file mode 100644 index 000000000..67794d26e --- /dev/null +++ b/tests/server_card_counter_test.cpp @@ -0,0 +1,149 @@ +/** @file server_card_counter_test.cpp + * @brief Tests for Server_Card counter operations. + * @ingroup Tests + */ + +#include +#include +#include +#include +#include + +TEST(ServerCardCounter, IncrementNewCounter) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + EXPECT_TRUE(card.incrementCounter(1, 10)); + EXPECT_EQ(card.getCounter(1), 10); +} + +TEST(ServerCardCounter, IncrementExistingCounter) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 50)); + EXPECT_TRUE(card.incrementCounter(1, 10)); + EXPECT_EQ(card.getCounter(1), 60); +} + +TEST(ServerCardCounter, IncrementOverflowProtection) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, std::numeric_limits::max())); + EXPECT_FALSE(card.incrementCounter(1, 1)); + EXPECT_EQ(card.getCounter(1), std::numeric_limits::max()); +} + +TEST(ServerCardCounter, DecrementUnderflowProtection) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, std::numeric_limits::min())); + EXPECT_FALSE(card.incrementCounter(1, -1)); + EXPECT_EQ(card.getCounter(1), std::numeric_limits::min()); +} + +TEST(ServerCardCounter, ReturnsFalseWhenUnchanged) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 50)); + EXPECT_FALSE(card.incrementCounter(1, 0)); + EXPECT_EQ(card.getCounter(1), 50); +} + +TEST(ServerCardCounter, DecrementToZeroRemovesCounter) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 10)); + EXPECT_TRUE(card.incrementCounter(1, -10)); + EXPECT_EQ(card.getCounter(1), 0); + EXPECT_FALSE(card.getCounters().contains(1)); +} + +TEST(ServerCardCounter, SetToZeroRemovesCounter) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 10)); + EXPECT_TRUE(card.setCounter(1, 0)); + EXPECT_EQ(card.getCounter(1), 0); + EXPECT_FALSE(card.getCounters().contains(1)); +} + +TEST(ServerCardCounter, SetCounterReturnsFalseWhenUnchanged) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 50)); + EXPECT_FALSE(card.setCounter(1, 50)); + EXPECT_EQ(card.getCounter(1), 50); +} + +TEST(ServerCardCounter, SetCounterReturnsTrueWhenChanged) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 50)); + EXPECT_TRUE(card.setCounter(1, 100)); + EXPECT_EQ(card.getCounter(1), 100); +} + +TEST(ServerCardCounter, SetCounterEventNotPopulatedWhenUnchanged) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 50)); + + Event_SetCardCounter event; + event.set_counter_id(999); + event.set_counter_value(999); + + EXPECT_FALSE(card.setCounter(1, 50, &event)); + EXPECT_EQ(event.counter_id(), 999); + EXPECT_EQ(event.counter_value(), 999); +} + +TEST(ServerCardCounter, IncrementCounterPopulatesEvent) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 50)); + + Event_SetCardCounter event; + EXPECT_TRUE(card.incrementCounter(1, 10, &event)); + + EXPECT_EQ(event.counter_id(), 1); + EXPECT_EQ(event.counter_value(), 60); +} + +TEST(ServerCardCounter, IncrementCounterEventReflectsClampedValue) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, std::numeric_limits::max() - 5)); + + Event_SetCardCounter event; + EXPECT_TRUE(card.incrementCounter(1, 10, &event)); + + EXPECT_EQ(event.counter_id(), 1); + EXPECT_EQ(event.counter_value(), std::numeric_limits::max()); +} + +TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 50)); + EXPECT_TRUE(card.incrementCounter(1, 10, nullptr)); + EXPECT_EQ(card.getCounter(1), 60); +} + +TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, std::numeric_limits::max())); + + Event_SetCardCounter event; + event.set_counter_id(999); + event.set_counter_value(999); + + EXPECT_FALSE(card.incrementCounter(1, 1, &event)); + EXPECT_EQ(event.counter_id(), 999); + EXPECT_EQ(event.counter_value(), 999); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tests/server_counter_test.cpp b/tests/server_counter_test.cpp new file mode 100644 index 000000000..0f41f2cbd --- /dev/null +++ b/tests/server_counter_test.cpp @@ -0,0 +1,86 @@ +/** @file server_counter_test.cpp + * @brief Tests for Server_Counter operations. + * @ingroup Tests + */ + +#include +#include +#include + +TEST(ServerCounter, IncrementDoesNotOverflow) +{ + Server_Counter c(1, "test", color(), 10, std::numeric_limits::max()); + bool changed = c.incrementCount(1); + EXPECT_FALSE(changed); + EXPECT_EQ(c.getCount(), std::numeric_limits::max()); +} + +TEST(ServerCounter, DecrementDoesNotUnderflow) +{ + Server_Counter c(1, "test", color(), 10, std::numeric_limits::min()); + bool changed = c.incrementCount(-1); + EXPECT_FALSE(changed); + EXPECT_EQ(c.getCount(), std::numeric_limits::min()); +} + +TEST(ServerCounter, SetCountReturnsFalseWhenUnchanged) +{ + Server_Counter c(1, "test", color(), 10, 50); + bool changed = c.setCount(50); + EXPECT_FALSE(changed); +} + +TEST(ServerCounter, IncrementReturnsChangeStatus) +{ + Server_Counter c(1, "test", color(), 10, 50); + EXPECT_TRUE(c.incrementCount(10)); + EXPECT_EQ(c.getCount(), 60); + EXPECT_FALSE(c.incrementCount(0)); + EXPECT_EQ(c.getCount(), 60); +} + +TEST(ServerCounter, LargePositiveDeltaDoesNotOverflow) +{ + Server_Counter c(1, "test", color(), 10, std::numeric_limits::max() - 10); + bool changed = c.incrementCount(std::numeric_limits::max()); + EXPECT_TRUE(changed); // Value changes from INT_MAX-10 to INT_MAX (clamped) + EXPECT_EQ(c.getCount(), std::numeric_limits::max()); +} + +TEST(ServerCounter, LargeNegativeDeltaDoesNotUnderflow) +{ + Server_Counter c(1, "test", color(), 10, std::numeric_limits::min() + 10); + bool changed = c.incrementCount(std::numeric_limits::min()); + EXPECT_TRUE(changed); // Value changes from INT_MIN+10 to INT_MIN (clamped) + EXPECT_EQ(c.getCount(), std::numeric_limits::min()); +} + +TEST(ServerCounter, SetCountReturnsTrueWhenChanged) +{ + Server_Counter c(1, "test", color(), 10, 50); + EXPECT_TRUE(c.setCount(100)); + EXPECT_EQ(c.getCount(), 100); +} + +TEST(ServerCounter, BasicIncrementWorks) +{ + Server_Counter c(1, "test", color(), 10, 50); + EXPECT_TRUE(c.incrementCount(10)); + EXPECT_EQ(c.getCount(), 60); + EXPECT_TRUE(c.incrementCount(-20)); + EXPECT_EQ(c.getCount(), 40); +} + +TEST(ServerCounter, MixedExtremesDoNotClamp) +{ + Server_Counter c(1, "test", color(), 10, std::numeric_limits::max()); + bool changed = c.incrementCount(std::numeric_limits::min()); + EXPECT_TRUE(changed); + EXPECT_EQ(c.getCount(), -1); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}