Centralize counter API with server-side bounds and no-op filtering (#6879)

* Refactor server counter API to own overflow protection and filter no-op events Counter modifications now clamp to int bounds server-side and return change status, allowing command handlers to skip network broadcasts when values don't actually change. * Centralize MAX_COUNTERS_ON_CARD and enforce [0, 999] bounds on server - Move MAX_COUNTERS_ON_CARD to trice_limits.h - Server clamps values in setCounter() and incrementCounter() - Client uses clamped comparison to allow recovery from invalid states - Add tests for clamping behavior * move incrementCount() implementation from header to cpp
2026-06-10 00:04:48 -07:00 · 2026-05-21 23:39:35 -04:00 · 2026-05-21 23:39:35 -04:00 · 8dca14933c
commit 8dca14933c
parent 74102aa1ec
12 changed files with 432 additions and 32 deletions
--- 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)
--- a/tests/server_card_counter_test.cpp
+++ b/tests/server_card_counter_test.cpp
@ -0,0 +1,183 @@
+/** @file server_card_counter_test.cpp
+ *  @brief Tests for Server_Card counter operations.
+ *  @ingroup Tests
+ */
+
+#include <gtest/gtest.h>
+#include <libcockatrice/network/server/remote/game/server_card.h>
+#include <libcockatrice/protocol/pb/event_set_card_counter.pb.h>
+#include <libcockatrice/utility/card_ref.h>
+#include <libcockatrice/utility/trice_limits.h>
+#include <limits>
+
+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, MAX_COUNTERS_ON_CARD));
+    EXPECT_FALSE(card.incrementCounter(1, 1));
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+}
+
+TEST(ServerCardCounter, DecrementUnderflowProtection)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    ASSERT_TRUE(card.setCounter(1, 5));
+    EXPECT_TRUE(card.incrementCounter(1, -10));
+    EXPECT_EQ(card.getCounter(1), 0);
+    EXPECT_FALSE(card.getCounters().contains(1));
+}
+
+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, MAX_COUNTERS_ON_CARD - 5));
+
+    Event_SetCardCounter event;
+    EXPECT_TRUE(card.incrementCounter(1, 10, &event));
+
+    EXPECT_EQ(event.counter_id(), 1);
+    EXPECT_EQ(event.counter_value(), MAX_COUNTERS_ON_CARD);
+}
+
+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, MAX_COUNTERS_ON_CARD));
+
+    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);
+}
+
+TEST(ServerCardCounter, SetCounterClampsNegativeToZero)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    EXPECT_FALSE(card.setCounter(1, -5));
+    EXPECT_EQ(card.getCounter(1), 0);
+    EXPECT_FALSE(card.getCounters().contains(1));
+}
+
+TEST(ServerCardCounter, SetCounterClampsAboveMaxToMax)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    EXPECT_TRUE(card.setCounter(1, 1500));
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+}
+
+TEST(ServerCardCounter, IncrementDoesNotGoBelowZero)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    ASSERT_TRUE(card.setCounter(1, 5));
+    EXPECT_TRUE(card.incrementCounter(1, -10));
+    EXPECT_EQ(card.getCounter(1), 0);
+    EXPECT_FALSE(card.getCounters().contains(1));
+}
+
+TEST(ServerCardCounter, IncrementDoesNotExceedMax)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5));
+    EXPECT_TRUE(card.incrementCounter(1, 10));
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}
--- a/tests/server_counter_test.cpp
+++ b/tests/server_counter_test.cpp
@ -0,0 +1,86 @@
+/** @file server_counter_test.cpp
+ *  @brief Tests for Server_Counter operations.
+ *  @ingroup Tests
+ */
+
+#include <gtest/gtest.h>
+#include <libcockatrice/network/server/remote/game/server_counter.h>
+#include <limits>
+
+TEST(ServerCounter, IncrementDoesNotOverflow)
+{
+    Server_Counter c(1, "test", color(), 10, std::numeric_limits<int>::max());
+    bool changed = c.incrementCount(1);
+    EXPECT_FALSE(changed);
+    EXPECT_EQ(c.getCount(), std::numeric_limits<int>::max());
+}
+
+TEST(ServerCounter, DecrementDoesNotUnderflow)
+{
+    Server_Counter c(1, "test", color(), 10, std::numeric_limits<int>::min());
+    bool changed = c.incrementCount(-1);
+    EXPECT_FALSE(changed);
+    EXPECT_EQ(c.getCount(), std::numeric_limits<int>::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<int>::max() - 10);
+    bool changed = c.incrementCount(std::numeric_limits<int>::max());
+    EXPECT_TRUE(changed); // Value changes from INT_MAX-10 to INT_MAX (clamped)
+    EXPECT_EQ(c.getCount(), std::numeric_limits<int>::max());
+}
+
+TEST(ServerCounter, LargeNegativeDeltaDoesNotUnderflow)
+{
+    Server_Counter c(1, "test", color(), 10, std::numeric_limits<int>::min() + 10);
+    bool changed = c.incrementCount(std::numeric_limits<int>::min());
+    EXPECT_TRUE(changed); // Value changes from INT_MIN+10 to INT_MIN (clamped)
+    EXPECT_EQ(c.getCount(), std::numeric_limits<int>::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<int>::max());
+    bool changed = c.incrementCount(std::numeric_limits<int>::min());
+    EXPECT_TRUE(changed);
+    EXPECT_EQ(c.getCount(), -1);
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}