diff --git a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt index 5840f80c0..2127487d2 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt @@ -3,6 +3,7 @@ set(CMAKE_AUTOUIC ON) set(CMAKE_AUTORCC ON) set(HEADERS + game/dump_zone_range.h game/server_abstract_participant.h game/server_abstract_player.h game/server_arrow.h @@ -33,6 +34,7 @@ qt6_wrap_cpp(MOC_SOURCES ${HEADERS}) add_library( libcockatrice_network_server_remote STATIC ${MOC_SOURCES} + game/dump_zone_range.cpp game/server_abstract_participant.cpp game/server_abstract_player.cpp game/server_arrow.cpp diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.cpp new file mode 100644 index 000000000..4ecca95cb --- /dev/null +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.cpp @@ -0,0 +1,17 @@ +#include "dump_zone_range.h" + +#include + +std::pair dumpZoneCardRange(qsizetype cardCount, int numberCards, bool isReversed) +{ + qsizetype count = 0; + if (numberCards == -1) { + count = cardCount; + } else if (numberCards > 0) { + count = std::min(numberCards, cardCount); + } + count = std::max(count, 0); + + const qsizetype begin = isReversed ? cardCount - count : 0; + return {begin, begin + count}; +} diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.h b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.h new file mode 100644 index 000000000..a688b09d0 --- /dev/null +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/dump_zone_range.h @@ -0,0 +1,29 @@ +#ifndef DUMP_ZONE_RANGE_H +#define DUMP_ZONE_RANGE_H + +#include +#include + +/** + * @brief Resolve a client's zone dump request against a zone's real card count. + * + * @c number_cards arrives from the client unvalidated. -1 means "the whole zone", which + * is the convention the client uses when sizing its own view of one. Every other negative + * value, and every value larger than the zone, has to be brought into range rather than + * used as an index: with a reversed request the zone offset is + * @c cardCount - @c numberCards, which runs off the front of the list when the request + * exceeds the zone and off the back when @c numberCards is -1. + * + * Kept in its own translation unit so it can be exercised without the rest of + * server_abstract_player.cpp, which refers to the RNG global that only the servatrice + * binary defines. + * + * @param cardCount number of cards the zone actually holds + * @param numberCards the requested count, -1 for the whole zone + * @param isReversed take the trailing cards instead of the leading ones + * @return the half-open index range [begin, end) into the zone's card list, always + * contained in [0, cardCount) + */ +std::pair dumpZoneCardRange(qsizetype cardCount, int numberCards, bool isReversed); + +#endif // DUMP_ZONE_RANGE_H 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 47c810d0d..c03f1e966 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 @@ -1,5 +1,6 @@ #include "server_abstract_player.h" +#include "dump_zone_range.h" #include "game/server_abstract_participant.h" #include "game/server_arrowtarget.h" #include "libcockatrice/protocol/pb/card_attributes.pb.h" @@ -1435,23 +1436,23 @@ Server_AbstractPlayer::cmdDumpZone(const Command_DumpZone &cmd, ResponseContaine int numberCards = cmd.number_cards(); const QList &cards = zone->getCards(); + const auto cardRange = dumpZoneCardRange(cards.size(), numberCards, cmd.is_reversed()); auto *re = new Response_DumpZone; ServerInfo_Zone *zoneInfo = re->mutable_zone_info(); zoneInfo->set_name(zone->getName().toStdString()); zoneInfo->set_type(zone->getType()); zoneInfo->set_with_coords(zone->hasCoords()); - zoneInfo->set_card_count(numberCards < cards.size() ? cards.size() : numberCards); + zoneInfo->set_card_count(cards.size()); - for (int i = 0; (i < cards.size()) && (i < numberCards || numberCards == -1); ++i) { - const auto &findId = cmd.is_reversed() ? cards.size() - numberCards + i : i; - Server_Card *card = cards[findId]; + for (qsizetype findId = cardRange.first; findId < cardRange.second; ++findId) { + Server_Card *card = cards.at(findId); QString displayedName = card->getFaceDown() ? QString() : card->getName(); ServerInfo_Card *cardInfo = zoneInfo->add_card_list(); cardInfo->set_provider_id(card->getProviderId().toStdString()); cardInfo->set_name(displayedName.toStdString()); if (zone->getType() == ServerInfo_Zone::HiddenZone) { - cardInfo->set_id(findId); + cardInfo->set_id(static_cast(findId)); } else { cardInfo->set_id(card->getId()); cardInfo->set_x(card->getX()); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 22dbedb98..9955e1189 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -206,6 +206,17 @@ cockatrice_add_test( LABELS server ) +cockatrice_add_test( + NAME + server_dump_zone_test + SOURCES + server_dump_zone_test.cpp + LIBS + libcockatrice_rng + libcockatrice_network + LABELS + server +) cockatrice_add_test( NAME server_counter_test diff --git a/tests/server_dump_zone_test.cpp b/tests/server_dump_zone_test.cpp new file mode 100644 index 000000000..71c410655 --- /dev/null +++ b/tests/server_dump_zone_test.cpp @@ -0,0 +1,125 @@ +/** @file server_dump_zone_test.cpp + * @brief Tests for resolving a client's zone dump request against a zone's card count. + * @ingroup Tests + */ + +#include +#include +#include +#include +#include + +namespace +{ +/// Indices a dump request resolves to, as the handler would walk them. +std::vector walked(qsizetype cardCount, int numberCards, bool isReversed) +{ + const auto [begin, end] = dumpZoneCardRange(cardCount, numberCards, isReversed); + std::vector out; + for (qsizetype i = begin; i < end; ++i) { + out.push_back(i); + } + return out; +} +} // namespace + +TEST(ServerDumpZone, ForwardTakesLeadingCards) +{ + EXPECT_EQ(walked(5, 2, false), (std::vector{0, 1})); +} + +TEST(ServerDumpZone, ForwardTakingAllCardsCoversTheZone) +{ + EXPECT_EQ(walked(3, 3, false), (std::vector{0, 1, 2})); +} + +TEST(ServerDumpZone, ForwardAskingForOneDumpsOne) +{ + EXPECT_EQ(walked(3, 1, false), (std::vector{0})); +} + +TEST(ServerDumpZone, MinusOneMeansTheWholeZone) +{ + EXPECT_EQ(walked(4, -1, false), (std::vector{0, 1, 2, 3})); +} + +TEST(ServerDumpZone, ReversedTakesTrailingCards) +{ + EXPECT_EQ(walked(5, 2, true), (std::vector{3, 4})); +} + +TEST(ServerDumpZone, ReversedAskingForOneTakesTheLast) +{ + EXPECT_EQ(walked(5, 1, true), (std::vector{4})); +} + +TEST(ServerDumpZone, ZeroDumpsNothing) +{ + EXPECT_TRUE(walked(5, 0, false).empty()); + EXPECT_TRUE(walked(5, 0, true).empty()); +} + +// A request for more cards than the zone holds is clamped rather than walked off the end. +// Forward, the old loop stopped at the zone's size on its own. +TEST(ServerDumpZone, ForwardOversizedRequestIsClamped) +{ + EXPECT_EQ(walked(3, 1000, false), (std::vector{0, 1, 2})); +} + +// Reversed, an oversized request put the offset at cardCount - numberCards, which is +// negative, so the walk started before the first card. +TEST(ServerDumpZone, ReversedOversizedRequestIsClamped) +{ + EXPECT_EQ(walked(3, 1000, true), (std::vector{0, 1, 2})); +} + +// Reversed, -1 put the offset at cardCount + 1, so the walk started one past the last card. +TEST(ServerDumpZone, ReversedWholeZoneRequestStaysInRange) +{ + EXPECT_EQ(walked(3, -1, true), (std::vector{0, 1, 2})); +} + +// Only -1 means "the whole zone". Any other negative count is not a request for anything, +// so it resolves to an empty range rather than to the whole zone. +TEST(ServerDumpZone, OtherNegativeCountsDumpNothing) +{ + EXPECT_TRUE(walked(5, -2, false).empty()); + EXPECT_TRUE(walked(5, -2, true).empty()); + EXPECT_TRUE(walked(5, INT_MIN, true).empty()); +} + +TEST(ServerDumpZone, EmptyZoneYieldsAnEmptyRange) +{ + EXPECT_TRUE(walked(0, -1, false).empty()); + EXPECT_TRUE(walked(0, -1, true).empty()); + EXPECT_TRUE(walked(0, 10, false).empty()); + EXPECT_TRUE(walked(0, 10, true).empty()); +} + +TEST(ServerDumpZone, NonPositiveCardCountYieldsAnEmptyRange) +{ + EXPECT_TRUE(walked(-1, -1, false).empty()); + EXPECT_TRUE(walked(-1, -1, true).empty()); +} + +// The invariant the handler depends on: whatever a client asks for, the indices it walks +// are a subset of the zone's cards and never step outside the list. number_cards is an +// unbounded client-supplied sint32, so this sweeps the edges rather than a chosen few. +TEST(ServerDumpZone, ResolvedRangeIsAlwaysWithinTheZone) +{ + const QList cardCounts = {0, 1, 2, 3, 7, 60, 1000000}; + const QList requests = {INT_MIN, -1000, -2, -1, 0, 1, 2, 3, 59, 60, 61, 999999, INT_MAX}; + + for (int cardCount : cardCounts) { + for (int request : requests) { + for (bool isReversed : {false, true}) { + const auto [begin, end] = dumpZoneCardRange(cardCount, request, isReversed); + SCOPED_TRACE(testing::Message() + << "cardCount=" << cardCount << " request=" << request << " isReversed=" << isReversed); + ASSERT_GE(begin, 0); + ASSERT_GE(end, begin); + ASSERT_LE(end, cardCount); + } + } + } +}