Add guild trophy ranking and caching#633
Conversation
- Add GuildTrophyRankInfo record as a DTO for guild ranking data. - Implement GameStorage methods: GetGuildTrophyRankInfo(by id/name), GetGuildIdByCharacterId, GetGuildTrophyRankings, and ComputeGuildTrophy. Rankings are computed by aggregating member achievement totals, ordered, and limited to top 200. Implement batch queries to reduce per-entity EF tracking. - Update WebController to expose GuildTrophy and PersonalGuildTrophy handlers, add a 3-minute in-memory cache for rankings, and use cached data when possible. Add necessary using and cache fields. - Change InGameRankPacket.GuildTrophy to accept a list of GuildTrophyRankInfo and serialize full guild info (rank, id, name, emblem, leader, and trophy breakdown). - Persist guild emblem updates in GuildManager by saving the guild to GameStorage after changes. These changes enable querying and serving guild trophy leaderboards and individual guild ranks with basic caching and saves emblems.
📝 WalkthroughWalkthroughAdds guild trophy ranking support: a new GuildTrophyRankInfo record, GameStorage methods to compute and rank guild trophies (batched aggregation), WebController endpoints and IMemoryCache usage (1-hour TTL), packet serialization for multiple guild trophy entries, and emblem persistence on update. Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebController
participant Cache
participant GameStorage
participant Packet
Client->>WebController: Request GuildTrophy(guildName) or PersonalGuildTrophy(characterId)
WebController->>Cache: Try get cached GuildTrophyRankings (1h TTL)
alt Cache Hit
Cache-->>WebController: Return cached GuildTrophyRankInfo[]
else Cache Miss
WebController->>GameStorage: GetGuildTrophyRankings() or GetGuildTrophyRankInfo(...)
GameStorage->>GameStorage: Batch fetch guilds, leaders, members, characters
GameStorage->>GameStorage: Aggregate AchievementInfo per guild (ComputeGuildTrophy)
GameStorage->>GameStorage: Sort, assign ranks, take top 200
GameStorage-->>WebController: Return ranked GuildTrophyRankInfo[]
WebController->>Cache: Store GuildTrophyRankInfo[] (1h TTL)
end
WebController->>Packet: GuildTrophy(rankInfos)
Packet->>Client: Serialized guild trophy ranking packet
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Maple2.Server.Web/Controllers/WebController.cs`:
- Around line 28-29: _guildTrophyCache and _guildTrophyCacheExpiry are static
shared fields accessed from concurrent request threads without synchronization;
protect all reads and writes by introducing a private static readonly lock
object (e.g. _guildTrophyCacheLock) and wrap any access/update logic that checks
expiry, reads, or populates _guildTrophyCache/_guildTrophyCacheExpiry in
lock(_guildTrophyCacheLock) blocks to ensure atomicity and memory visibility;
apply the same locking around the other access site mentioned (lines ~341-350)
so all cache operations use the same lock.
🧹 Nitpick comments (3)
Maple2.Database/Storage/Game/GameStorage.Web.cs (2)
101-138:GetGuildTrophyRankInfo(long)recomputes all guild trophies — consider using the cached rankings instead.This method calls
ComputeGuildTrophyfor every guild (line 120-123) just to determine the rank of a single guild. SinceWebControlleralready caches the full rankings viaGetCachedGuildTrophyRankings(), this expensive per-guild method is only reached as a DB fallback in theGuildTrophy(userName)search path (WebController line 321). For a guild not in the top-200 cache, this triggers a full recomputation.If this is acceptable as POC (per the comment on line 11), consider at least adding a similar comment here to flag it for future optimization.
159-193:GetGuildTrophyRankingshas O(guilds × members) DB round-trips viaComputeGuildTrophy.Each call to
ComputeGuildTrophy(line 174) issues two queries (members + characters) plus oneGetAchievementInfocall per member. With many guilds and members, this is a significant DB load. The 3-minute cache inWebControllermitigates this, but a single cache-miss still triggers a heavy burst.This is consistent with the existing character trophy approach, so fine for POC — just noting it for awareness.
Maple2.Server.Web/Controllers/WebController.cs (1)
310-326: Cache miss for a guild name search still triggers a full recomputation.When
userNameis provided and the guild isn't found in the cached top-200 rankings (line 314-318), the fallbackdb.GetGuildTrophyRankInfo(userName)(line 321) recomputes trophies for all guilds to determine the rank of a single guild. This is expected for guilds outside the top 200, but worth noting as a potential abuse vector if users can trigger arbitrary guild-name searches.
Introduce IMemoryCache to cache ranking responses and reduce DB load. WebController now accepts IMemoryCache, removes the old static guild-trophy cache, and caches Trophy, PersonalTrophy, GuildTrophy, PersonalGuildTrophy and full guild rankings (GetCachedGuildTrophyRankings) using hourly TTL (RankingCacheDuration). Program.cs registers AddMemoryCache so the cache is available via DI. Cache keys follow patterns like "Trophy_{user}", "PersonalTrophy_{id}", "GuildTrophy_{name}", "PersonalGuildTrophy_{id}", and "GuildTrophyRankings".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Maple2.Server.Web/Controllers/WebController.cs`:
- Around line 289-310: The cached ByteWriter instances in Trophy (and similarly
in PersonalTrophy, GuildTrophy, PersonalGuildTrophy) are mutable and may be
mutated by callers; instead cache an immutable serialized form (e.g., call
ZlibCompress(writer) or writer.ToArray() and cache the resulting byte[]), and on
each request reconstruct a fresh ByteWriter from that byte[] before returning;
update the usage around InGameRankPacket.Trophy, cache.Set(cacheKey, ... ,
RankingCacheDuration) and cache.TryGetValue to store/retrieve byte[] (or
compressed bytes) and wrap them into a new ByteWriter for the response so the
cached data cannot be mutated.
🧹 Nitpick comments (3)
Maple2.Server.Web/Controllers/WebController.cs (3)
28-34: Constructor injection used instead of property injection pattern.The coding guidelines specify using the property injection pattern with
requiredproperties for dependency injection in service classes. The newcachefield follows the existing constructor-injection pattern in this class, but if you plan to align with the guidelines, all three dependencies should be converted.Example property injection pattern
- private readonly WebStorage webStorage; - private readonly GameStorage gameStorage; - private readonly IMemoryCache cache; - private static readonly TimeSpan RankingCacheDuration = TimeSpan.FromHours(1); - - public WebController(WebStorage webStorage, GameStorage gameStorage, IMemoryCache cache) { - this.webStorage = webStorage; - this.gameStorage = gameStorage; - this.cache = cache; - } + public required WebStorage webStorage { private get; init; } + public required GameStorage gameStorage { private get; init; } + public required IMemoryCache cache { private get; init; } + private static readonly TimeSpan RankingCacheDuration = TimeSpan.FromHours(1);As per coding guidelines: "Use property injection pattern with required properties for dependency injection in service classes".
349-349: UncachedByteWriterfor the "all guilds" path.When
userNameis empty,GuildTrophybuilds a newByteWriterfromGetCachedGuildTrophyRankings()on every request without caching the serialized result. All other ranking paths cache theirByteWriter. This means every "list all" request re-serializes the full rankings list.Suggested fix
- return InGameRankPacket.GuildTrophy(GetCachedGuildTrophyRankings()); + const string allCacheKey = "GuildTrophy_all"; + if (!cache.TryGetValue(allCacheKey, out ByteWriter? allResult)) { + allResult = InGameRankPacket.GuildTrophy(GetCachedGuildTrophyRankings()); + cache.Set(allCacheKey, allResult, RankingCacheDuration); + } + return allResult!;
352-371: Guilds outside top 200 silently report rank 0.When a character's guild isn't in the cached top-200 rankings (line 363),
cached?.Rank ?? 0returns 0. Unlike theGuildTrophysearch path (which falls back todb.GetGuildTrophyRankInfo), this path has no DB fallback to compute the exact rank. If this is intentional (unranked guilds show rank 0), a brief comment would clarify.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes