From 695642a91fad20b418e6b817a08217c9161a158a Mon Sep 17 00:00:00 2001 From: lenisko <10072920+lenisko@users.noreply.github.com> Date: Fri, 6 Feb 2026 02:54:41 +0100 Subject: [PATCH 1/3] fix: uncomment and fix CalculateTopRanks, optimize core functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CalculateTopRanks: - Fix lastRank tracking: populate lastRank[i] so duplicate detection works across level caps - Fix Capped mutation: use index-based tracking (lastRankIdx) to modify rankings directly instead of local copies - Pass comparator to calculateRanksCompact; pass stats as pointer - Replace O(n²) Capped propagation with O(n) index lookup Optimizations: - Simplify RankingComparatorPreferHigherCp/PreferLowerCp with early return - Replace reflect.DeepEqual with sha256 hash comparison in WatchPokemonData Cleanup: - Remove "Current State" section from README - Remove commented-out CalculateAllRanks test/benchmark --- README.md | 5 --- ohbem.go | 118 +++++++++++++++++++++++++------------------------- ohbem_test.go | 47 -------------------- pvp_core.go | 30 +++++-------- 4 files changed, 69 insertions(+), 131 deletions(-) diff --git a/README.md b/README.md index bd4b4b5..bd84c1c 100644 --- a/README.md +++ b/README.md @@ -20,11 +20,6 @@ This is a rewrite of node version https://github.com/Mygod/ohbem * Optional built-in caching * Faster than node :) -## Current State - -- `CalculateTopRanks` is broken. -- Everything else is fine. - ## [Documentation](https://pkg.go.dev/github.com/UnownHash/gohbem) ## Usage diff --git a/ohbem.go b/ohbem.go index dfef4c7..33aa272 100644 --- a/ohbem.go +++ b/ohbem.go @@ -1,11 +1,11 @@ package gohbem import ( + "crypto/sha256" "encoding/json" "fmt" "math" "os" - "reflect" "sort" "sync" "time" @@ -88,7 +88,17 @@ func (o *Ohbem) WatchPokemonData() error { o.log("Remote MasterFile fetch failed") continue } - if reflect.DeepEqual(o.PokemonData, pokemonData) { + newData, hashErr := json.Marshal(pokemonData) + if hashErr != nil { + o.log("Remote MasterFile hash failed") + continue + } + oldData, hashErr := json.Marshal(o.PokemonData) + if hashErr != nil { + o.log("Current MasterFile hash failed") + continue + } + if sha256.Sum256(newData) == sha256.Sum256(oldData) { continue } else { o.log("New MasterFile found! Updating PokemonData") @@ -177,28 +187,6 @@ func (o *Ohbem) calculateAllRanksCompact(stats *PokemonStats, cpCap int) (map[in return result, filled } -/* -// CalculateAllRanks Calculate all PvP ranks for a specific base stats with the specified CP cap. -func (o *Ohbem) CalculateAllRanks(stats PokemonStats, cpCap int) (map[int][16][16][16]Ranking, bool) { - filled := false - result := make(map[int][16][16][16]Ranking) - - for _, lvCap := range o.LevelCaps { - lvCapFloat := float64(lvCap) - if !o.IncludeHundosUnderCap && calculateCp(stats, 15, 15, 15, lvCapFloat) <= cpCap { - continue - } - result[lvCap], _ = calculateRanks(stats, cpCap, lvCapFloat) - filled = true - if calculateCp(stats, 0, 0, 0, lvCapFloat+0.5) > cpCap { - break - } else { - result[MaxLevel], _ = calculateRanks(stats, cpCap, float64(MaxLevel)) - } - } - return result, filled -} - // CalculateTopRanks Return ranked list of PVP statistics for a given Pokémon. func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolution int, ivFloor int) (map[string][]Ranking, error) { result := make(map[string][]Ranking) @@ -208,14 +196,12 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut } masterPokemon := o.PokemonData.Pokemon[pokemonId] - var stats PokemonStats - var masterForm Form - var masterEvolution PokemonStats if masterPokemon.Attack == 0 { return result, nil } + var masterForm Form if _, ok := masterPokemon.Forms[form]; ok && form != 0 { masterForm = masterPokemon.Forms[form] } else { @@ -227,6 +213,7 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut } } + var masterEvolution PokemonStats if _, ok := masterForm.TempEvolutions[evolution]; ok && evolution != 0 { masterEvolution = masterForm.TempEvolutions[evolution] } else { @@ -237,41 +224,45 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut } } + var stats PokemonStats if masterEvolution.Attack != 0 { + stats = masterEvolution + } else if masterForm.Attack != 0 { stats = PokemonStats{ - Attack: masterEvolution.Attack, - Defense: masterEvolution.Defense, - Stamina: masterEvolution.Stamina, + Attack: masterForm.Attack, + Defense: masterForm.Defense, + Stamina: masterForm.Stamina, } } else { - if masterForm.Attack != 0 { - stats = PokemonStats{ - Attack: masterForm.Attack, - Defense: masterForm.Defense, - Stamina: masterForm.Stamina, - } - } else { - stats = PokemonStats{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - } + stats = PokemonStats{ + Attack: masterPokemon.Attack, + Defense: masterPokemon.Defense, + Stamina: masterPokemon.Stamina, } } + if o.RankingComparator == nil { + o.RankingComparator = RankingComparatorDefault + } + for leagueName, leagueOptions := range o.Leagues { - var rankings, lastRank []Ranking - var lastStat Ranking + var rankings []Ranking + lastRank := make([]Ranking, 0) + lastRankIdx := make([]int, 0) // indices into rankings slice for O(1) Capped updates processLevelCap := func(lvCap float64, setOnDup bool) { - combinations, sortedRanks := calculateRanksCompact(stats, leagueOptions.Cap, lvCap, ivFloor) + combinations, sortedRanks := calculateRanksCompact(&stats, leagueOptions.Cap, lvCap, o.RankingComparator, ivFloor) - for i := 0; i < len(sortedRanks); i++ { + for i := 0; i < 4096; i++ { stat := &sortedRanks[i] + if stat.Value == 0 { + break + } rank := combinations[stat.Index] if rank > maxRank { for len(lastRank) > i { lastRank = lastRank[:len(lastRank)-1] + lastRankIdx = lastRankIdx[:len(lastRankIdx)-1] } break } @@ -279,16 +270,19 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut defense := stat.Index >> 4 % 16 stamina := stat.Index % 16 - if len(lastRank) > i { - lastStat = lastRank[i] + var lastStat *Ranking + if i < len(lastRank) { + lastStat = &lastRank[i] } - if lastStat.Value != 0 && stat.Level == lastStat.Level && rank == lastStat.Rank && attack == lastStat.Attack && defense == lastStat.Defense && stamina == lastStat.Stamina { + if lastStat != nil && stat.Level == lastStat.Level && rank == lastStat.Rank && + attack == lastStat.Attack && defense == lastStat.Defense && + stamina == lastStat.Stamina { if setOnDup { - lastStat.Capped = true + rankings[lastRankIdx[i]].Capped = true } } else if !setOnDup { - lastStat = Ranking{ + entry := Ranking{ Rank: rank, Attack: attack, Defense: defense, @@ -299,7 +293,14 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut Cp: stat.Cp, Percentage: roundFloat(stat.Value/sortedRanks[0].Value, 5), } - rankings = append(rankings, lastStat) + rankingsIdx := len(rankings) + rankings = append(rankings, entry) + for len(lastRank) <= i { + lastRank = append(lastRank, Ranking{}) + lastRankIdx = append(lastRankIdx, 0) + } + lastRank[i] = entry + lastRankIdx[i] = rankingsIdx } } } @@ -309,9 +310,9 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut } else if leagueName == "master" { for _, lvCap := range o.LevelCaps { lvCapFloat := float64(lvCap) - maxHp := calculateHp(stats, 15, lvCapFloat) + maxHp := calculateHp(&stats, 15, lvCapFloat) for stamina := ivFloor; stamina < 15; stamina++ { - if calculateHp(stats, stamina, lvCapFloat) == maxHp { + if calculateHp(&stats, stamina, lvCapFloat) == maxHp { entry := Ranking{ Attack: 15, Defense: 15, @@ -328,14 +329,14 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut maxed := false for _, lvCap := range o.LevelCaps { lvCapFloat := float64(lvCap) - if !o.IncludeHundosUnderCap && calculateCp(stats, 15, 15, 15, lvCapFloat) <= leagueOptions.Cap { + if !o.IncludeHundosUnderCap && calculateCp(&stats, 15, 15, 15, lvCapFloat) <= leagueOptions.Cap { continue } processLevelCap(lvCapFloat, false) - if calculateCp(stats, ivFloor, ivFloor, ivFloor, lvCapFloat+0.5) > leagueOptions.Cap { + if calculateCp(&stats, ivFloor, ivFloor, ivFloor, lvCapFloat+0.5) > leagueOptions.Cap { maxed = true - for ix := range lastRank { - lastRank[ix].Capped = true + for _, idx := range lastRankIdx { + rankings[idx].Capped = true } break } @@ -351,7 +352,6 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut return result, nil } -*/ // CalculateCp calculates CP for your pokemon. Errors if pokemon cannot be found in master. func (o *Ohbem) CalculateCp(pokemonId, form, evolution, attack, defense, stamina int, level float64) (int, error) { diff --git a/ohbem_test.go b/ohbem_test.go index 926aae8..db620b6 100644 --- a/ohbem_test.go +++ b/ohbem_test.go @@ -82,52 +82,6 @@ func BenchmarkCalculateAllRanksCompactCached(b *testing.B) { } } -/* -func TestCalculateAllRanks(t *testing.T) { - ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} - err := ohbem.LoadPokemonData("./test/master-test.json") - if err != nil { - t.Errorf("can't load MasterFile") - } - - var tests = []struct { - stats PokemonStats - level int - cpCap int - a int - d int - s int - outValue float64 - outLevel float64 - outCp int - outPercentage float64 - outRank int16 - }{ - {PikachuStats, 50, 300, 0, 0, 0, 155813.01965332002, 14.5, 299, 0.93235, 1105}, - } - - for ix, test := range tests { - testName := fmt.Sprintf("%d", ix) - t.Run(testName, func(t *testing.T) { - combinations, _ := ohbem.CalculateAllRanks(PikachuStats, test.cpCap) - ans := combinations[test.level][test.a][test.d][test.s] - if ans.Value != test.outValue || ans.Level != test.outLevel || ans.Cp != test.outCp || ans.Rank != test.outRank { - t.Errorf("got %+v, want %+v", ans, test) - } - }) - } -} - -func BenchmarkCalculateAllRanks(b *testing.B) { - ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} - _ = ohbem.LoadPokemonData("./test/master-test.json") - - b.ResetTimer() - for i := 0; i < b.N; i++ { - _, _ = ohbem.CalculateAllRanks(PikachuStats, 5000) - } -} - func TestCalculateTopRanks(t *testing.T) { ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} err := ohbem.LoadPokemonData("./test/master-test.json") @@ -180,7 +134,6 @@ func BenchmarkCalculateTopRanks(b *testing.B) { _, _ = ohbem.CalculateTopRanks(500, 257, 0, 0, 1) } } -*/ func TestOhbem_CalculateCp(t *testing.T) { ohbem := Ohbem{} diff --git a/pvp_core.go b/pvp_core.go index 019b7fd..5b02f3a 100644 --- a/pvp_core.go +++ b/pvp_core.go @@ -137,18 +137,13 @@ func RankingComparatorDefault(a, b *PvPRankingStats) int { // While ties are not meaningfully different most of the time, // the rationale here is that a higher CP looks more intimidating. func RankingComparatorPreferHigherCp(a, b *PvPRankingStats) int { - d := RankingComparatorDefault(a, b) - if d > 0 { - return 1 - } - if d < 0 { - return -1 + if d := RankingComparatorDefault(a, b); d != 0 { + return d } - d = b.Cp - a.Cp - if d > 0 { + switch { + case b.Cp > a.Cp: return 1 - } - if d < 0 { + case b.Cp < a.Cp: return -1 } return 0 @@ -158,18 +153,13 @@ func RankingComparatorPreferHigherCp(a, b *PvPRankingStats) int { // While ties are not meaningfully different most of the time, // the rationale here is that you can flex beating your opponent using one with a lower CP. func RankingComparatorPreferLowerCp(a, b *PvPRankingStats) int { - d := RankingComparatorDefault(a, b) - if d > 0 { - return 1 - } - if d < 0 { - return -1 + if d := RankingComparatorDefault(a, b); d != 0 { + return d } - d = a.Cp - b.Cp - if d > 0 { + switch { + case a.Cp > b.Cp: return 1 - } - if d < 0 { + case a.Cp < b.Cp: return -1 } return 0 From 9464dd4e82457619720b8ef3d4e3b736f38b5d1a Mon Sep 17 00:00:00 2001 From: lenisko <10072920+lenisko@users.noreply.github.com> Date: Tue, 5 May 2026 14:47:40 +0200 Subject: [PATCH 2/3] fix: address review punch-list (races, bugs, perf, tests) - security/concurrency: add HTTP timeout + LimitReader + status check on fetchMasterFile; reuse package-level http.Client; add sync.RWMutex on Ohbem; swap compactRankCache to atomic.Pointer[sync.Map]; restartable watcher (nil channel after Stop, double-stop returns ErrNilChannel); watcher saves to tmp+rename before in-memory swap - correctness: fix FindBaseStats else-clobber, FilterLevelCaps value-copy mutate, CalculateTopRanks master-league stamina<=15; add IV/level validation to CalculateCp; rename IsMegaUnreleased arg to tempEvolution; bound recursion depth in QueryPvPRank - perf: bit-pack cacheKey; sync.Pool for the 16 KiB rank arena; slices.SortFunc replaces sort.Sort interface; iterate LevelCaps directly in QueryPvPRank (no per-call sort+alloc); roundFloat fast path for precision=5; hoist RankingComparator default into Load/Fetch - cleanup: extract resolveStats helper; replace parallel slices in CalculateTopRanks with single struct slice; drop dead commented calculateRanks blocks; regroup errors.go; remove unused Ranking.Index - api: add Ohbem.MasterFileURL override; bump VERSION to 0.13.0 - tests/CI: re-enable master-league CalculateTopRanks cases; add TestFilterLevelCapsMerge, TestWatchPokemonDataRestart, FetchPokemonData error paths via httptest, atomic Save round-trip, FuzzQueryPvPRankBounds and FuzzCalculateCpBounds; CI now runs go test -race - docs: drop "broken" tag on CalculateTopRanks; document MasterFileURL; add DETAIL_REVIEW.md status table; add Makefile --- .github/workflows/test.yml | 2 +- DETAIL_REVIEW.md | 434 ++++++++++++++++++++++++++++++++ Makefile | 51 ++++ README.md | 4 +- errors.go | 68 ++--- ohbem.go | 491 ++++++++++++++++++------------------- ohbem_test.go | 133 +++++++++- pvp_core.go | 103 +++----- pvp_core_test.go | 83 ------- structs.go | 13 +- utils.go | 50 ++-- 11 files changed, 976 insertions(+), 456 deletions(-) create mode 100644 DETAIL_REVIEW.md create mode 100644 Makefile diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 198525e..46e2218 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,4 +29,4 @@ jobs: run: go vet . - name: Test - run: go test -bench=. -benchmem -v . + run: go test -race -bench=. -benchmem -v . diff --git a/DETAIL_REVIEW.md b/DETAIL_REVIEW.md new file mode 100644 index 0000000..1f654fd --- /dev/null +++ b/DETAIL_REVIEW.md @@ -0,0 +1,434 @@ +# Gohbem — Detailed Code Review + +Scope: full source review (`ohbem.go`, `pvp_core.go`, `structs.go`, `utils.go`, `errors.go`, `cpm.go`, `cpm_test.go`, `ohbem_test.go`). Findings grouped by severity. Each item cites `file:line`. + +Repo version: `0.12.0` (per `ohbem.go:18`). + +--- + +## 1. Security + +### 1.1 No HTTP timeout on `fetchMasterFile` — DoS / hang risk *(high)* +`utils.go:34` — `client := &http.Client{}` has no `Timeout`. A slow or stalled remote (`raw.githubusercontent.com`) can hang the calling goroutine indefinitely. Inside `WatchPokemonData` the watcher goroutine will block on `client.Do`, missing future tick events; if called synchronously by user code it can hang the server. + +**Fix**: +```go +client := &http.Client{Timeout: 30 * time.Second} +``` +Better: build a single package-level `*http.Client` reused across calls (avoid per-call TCP/TLS pool churn). + +### 1.2 Unbounded response body — memory exhaustion *(medium)* +`utils.go:43` — `json.NewDecoder(resp.Body).Decode(&data)` reads without a size cap. A hostile or compromised endpoint can stream gigabytes. Wrap with `io.LimitReader`: +```go +err = json.NewDecoder(io.LimitReader(resp.Body, 32<<20)).Decode(&data) +``` +(MasterFile is ~1–2 MiB; 32 MiB is generous.) + +### 1.3 No HTTP status check *(medium)* +`utils.go:35` — non‑2xx responses (404, 503, captive‑portal HTML, GitHub rate limit) flow into `Decode`, returning the generic `ErrMasterFileDecode` and masking the real failure. Add: +```go +if resp.StatusCode != http.StatusOK { + return PokemonData{}, ErrMasterFileFetch +} +``` + +### 1.4 `LoadPokemonData` reads arbitrary file with `os.ReadFile` *(low)* +`ohbem.go:34` — entire file slurped into memory; a maliciously huge JSON triggers OOM. Streaming via `os.Open` + `json.NewDecoder` with `LimitReader` mitigates. Acceptable for trusted callers but worth documenting. + +### 1.5 `SavePokemonData` writes mode `0644` *(low)* +`ohbem.go:52` — world‑readable. The data is not sensitive, but if the path is shared, prefer `0600`. Also `os.WriteFile` is non-atomic; a crash mid-write leaves a corrupt cache file. Use temp-file + `os.Rename` for atomic replace: +```go +tmp := filePath + ".tmp" +if err := os.WriteFile(tmp, data, 0600); err != nil { return ErrMasterFileSave } +return os.Rename(tmp, filePath) +``` + +### 1.6 Data races on `Ohbem` shared state *(high)* +Multiple goroutines (caller code + watcher) read/write the same fields with no synchronization: + +- `ohbem.go:105` watcher goroutine writes `o.PokemonData = pokemonData` while query methods (`QueryPvPRank`, `CalculateTopRanks`, `FindBaseStats`, `IsMegaUnreleased`, `CalculateCp`) read it concurrently. Race per `go vet -race`. +- `ohbem.go:150,245` lazy default `o.RankingComparator = RankingComparatorDefault` is a write from any caller goroutine. Concurrent writes of identical pointer are still races under the Go memory model. +- `ohbem.go:135` `o.compactRankCache = sync.Map{}` reassigns a `sync.Map` value (`structs.go:18`). Copying / replacing `sync.Map` while another goroutine calls `Load`/`Store` corrupts internal state. `go vet` flags `sync.Map` value copy. +- `ohbem.go:41,106` `o.PokemonData.Initialized = true` written without atomicity; `safetyCheck` reads it. + +**Fix**: +- Add `sync.RWMutex` to `Ohbem`; take read lock in query methods, write lock in `Watch`/`Load`/`Fetch`/`ClearCache`. +- Change `compactRankCache sync.Map` → `compactRankCache *sync.Map` so it can be atomically swapped via `atomic.Pointer[sync.Map]`. +- Move `RankingComparator` default into the constructor (see §3.2) so the field is never written after init. + +### 1.7 Watcher cannot be restarted *(medium)* +`ohbem.go:124` — `StopWatchingPokemonData` closes `o.watcherChan` but never sets it to `nil`. Subsequent `WatchPokemonData` returns `ErrWatcherStarted` (`ohbem.go:60`). Also a second `StopWatchingPokemonData` call would `close` a closed channel and panic. Fix: +```go +func (o *Ohbem) StopWatchingPokemonData() error { + if o.watcherChan == nil { return ErrNilChannel } + close(o.watcherChan) + o.watcherChan = nil + return nil +} +``` + +### 1.8 `errors.go` exports mutable error vars *(informational)* +`errors.go:6+` — `var Err… = errors.New(...)` is the standard Go pattern but allows callers to reassign. If you want immutability use `errors.New` returns wrapped behind a function, or accept the convention. + +### 1.9 `User-Agent` header concatenation *(informational)* +`utils.go:32` — `fmt.Sprintf("Gohbem/%s", VERSION)` with `VERSION` const is safe today; if `VERSION` ever derives from user/runtime input, sanitize for CR/LF to prevent header injection. + +--- + +## 2. Correctness / Logic Bugs + +### 2.1 `FindBaseStats` overwrites `masterForm` in the wrong else *(bug, high)* +`ohbem.go:602–610`: +```go +if _, ok := masterPokemon.TempEvolutions[evolution]; ok && evolution != 0 { + masterEvolution = masterPokemon.TempEvolutions[evolution] +} else { + masterForm = Form{Attack: masterPokemon.Attack, ...} // <-- clobbers form +} +``` +The `else` branch resets `masterForm` to base stats even when the form *was* found earlier (lines 592–600). This silently strips per-form base stats whenever `evolution` is unset or unknown. Tests pass because none of the tested cases hit a "form found, evolution missing" path with mismatched form/base stats. + +**Fix**: drop the entire else branch; `masterEvolution` remains zero and the existing fallthrough at lines 612–626 already handles it: +```go +if me, ok := masterPokemon.TempEvolutions[evolution]; ok && evolution != 0 { + masterEvolution = me +} +``` + +Also note: this lookup ignores `masterForm.TempEvolutions` even though `Form` has its own (`structs.go:107`). Should mirror `QueryPvPRank` / `CalculateCp`: try `masterForm.TempEvolutions[evolution]` first, then fall back to `masterPokemon.TempEvolutions[evolution]`. + +### 2.2 `FilterLevelCaps` mutates a value copy, not the result slice *(bug, high)* +`ohbem.go:644–673`: +```go +last = result[len(result)-1] // copy +... +if last.Pokemon != 0 && last.Pokemon == entry.Pokemon ... { + last.Cap = entry.Cap // <-- writes to local copy only + if entry.Capped { last.Capped = true } +} +``` +`last` is a `PokemonEntry` value, not a pointer. The merge mutations (`last.Cap`, `last.Capped`) never propagate back into `result`. The collapsed entry keeps the *first* `Cap` it saw and never inherits `Capped=true` from a later level cap. Tests assert only `len(output)` so the bug is invisible. + +**Fix**: +```go +ref := &result[len(result)-1] +if ref.Pokemon != 0 && ref.Pokemon == entry.Pokemon && ... { + ref.Cap = entry.Cap + if entry.Capped { ref.Capped = true } +} else { + result = append(result, entry) +} +``` +And drop the separate `last` var entirely — re-derive from `result` each iteration. + +### 2.3 `CalculateTopRanks` master league: missing 15/15/15 *(bug, medium)* +`ohbem.go:314` — `for stamina := ivFloor; stamina < 15; stamina++`. The loop excludes `stamina == 15`, so the canonical 15/15/15 entry is never produced for the master league. Compare with `QueryPvPRank` (`ohbem.go:496`) where the special path is gated by `stamina < 15` *because* the trivial 15/15/15 case is handled elsewhere — but in `CalculateTopRanks` there is no elsewhere. + +Verify against tests; the master test cases for `CalculateTopRanks` are commented out (`ohbem_test.go:110–111`), so this is uncovered. + +**Fix**: change the bound to `<= 15` (the inner equality check `calculateHp(...,stamina,...) == maxHp` is true at stamina==15 trivially, so it self-handles). + +### 2.4 `cacheKey` magic radix and overflow risk *(low)* +`ohbem.go:142`: +```go +cacheKey := int64(cpCap*999*999*999 + stats.Attack*999*999 + stats.Defense*999 + stats.Stamina) +``` +The expression is evaluated in `int` (platform-dependent: 32-bit on 386/arm). On 32-bit platforms, `cpCap*999*999*999` overflows for any `cpCap >= 2`. Even on 64-bit, `999` is a strange radix — if a stat ever reaches `999` (unlikely but unguarded) collisions silently corrupt cache hits. + +**Fix**: use bit packing — Pokémon stats fit comfortably in 16 bits, `cpCap` in ~14: +```go +cacheKey := int64(cpCap)<<48 | int64(stats.Attack)<<32 | int64(stats.Defense)<<16 | int64(stats.Stamina) +``` +Faster (no multiplications), no overflow, no collision. + +### 2.5 `WatchPokemonData` writes new data even when save fails *(logic)* +`ohbem.go:105–115`: `o.PokemonData = pokemonData` happens *before* the cache file save. If the save fails, in-memory data is the new version but the cache file still points at the old one. On next process start, `LoadPokemonData(MasterFileCachePath)` loads stale data while the service had been running on fresh data. Either: +- Save first, swap second, or +- Log loudly and accept (current behavior with no swap rollback is fine if documented). + +### 2.6 `calculateCpMultiplier` half-level branch — verify rounding *(needs validation, low)* +`pvp_core.go:15–20`: +```go +baseCpm := float64(float32(0.5903 + float64(baseLevel)*0.005)) +... +nextCpm := float64(float32(0.5903 + float64(baseLevel+1)*0.005)) +return math.Sqrt((baseCpm*baseCpm + nextCpm*nextCpm) / 2) +``` +Used only for level > 55 (which the masterfile multiplier table doesn’t supply). The double `float64(float32(...))` is intentional (Niantic’s client uses single-precision multipliers), but Levels 1–55 use the precomputed table from `cpm.go` and Levels >55 do not exist in current Pokémon GO (caps are 50/51). Unreachable code; consider removing or asserting. + +### 2.7 `calculatePvPStat` binary search midpoint *(verify)* +`pvp_core.go:52`: +```go +mid := math.Ceil(lowest+highest) / 2 +``` +Reads as `Ceil(sum) / 2`, not `Ceil(sum/2)`. Because `lowest`/`highest` step in 0.5, `lowest+highest` is always a multiple of 0.5, so `Ceil` only acts on `*.5` sums. The result still lands on a 0.5 grid. Behavior matches existing tests, but the parenthesization is non-obvious — add `(lowest+highest)/2` rounded to nearest 0.5 with explicit helper, or comment why. + +### 2.8 `IsMegaUnreleased` parameter naming mismatch *(doc)* +`ohbem.go:630` — second parameter is named `evolution` but tests pass *form* values (`{150, 2, true}` is Mewtwo Mega-Y). Rename to clarify or align with TempEvolution map keys. + +### 2.9 `QueryPvPRank` recursion has no cycle guard *(defensive)* +`ohbem.go:553` recursively calls `o.QueryPvPRank` for each `evolution.Pokemon`. If MasterFile data is ever malformed (cyclic evolutions), this stack‑overflows. Track a small visited set on the stack or cap recursion depth. + +### 2.10 Fields written via positional struct literal *(fragile)* +`ohbem.go:525,527`: +```go +pushAllEntries(&PokemonStats{masterForm.Attack, masterForm.Defense, masterForm.Stamina, false}, 0) +``` +Positional init breaks silently if `PokemonStats` field order changes (`structs.go:119`). Use named fields: +```go +&PokemonStats{Attack: masterForm.Attack, Defense: masterForm.Defense, Stamina: masterForm.Stamina} +``` +(`Unreleased` defaults to false anyway.) + +### 2.11 `CalculateCp` doesn’t validate IV/level range *(consistency)* +`ohbem.go:357` — unlike `QueryPvPRank` (`ohbem.go:400`), `CalculateCp` accepts negative IVs, IVs > 15, and level < 1 silently. Either validate consistently or document the divergence. + +--- + +## 3. Optimizations + +### 3.1 Hash → byte compare in watcher *(easy win)* +`ohbem.go:91–101` marshals both old and new payloads then SHA-256s them. SHA‑256 is unnecessary; equal bytes is sufficient and twice as fast: +```go +if bytes.Equal(newData, oldData) { continue } +``` +Even better: cache the previous marshalled bytes (or its hash) on the `Ohbem` struct so each tick only marshals the *new* fetched payload. Saves ~50% of the work per tick. + +Drop the unused import `crypto/sha256` afterwards. + +### 3.2 Lazy `RankingComparator` default — hoist to constructor *(perf + race fix)* +`ohbem.go:149,244` runs the nil check on every call. Set once in `FetchPokemonData`/`LoadPokemonData` (or expose `NewOhbem(...)` factory). Eliminates the per-call branch *and* fixes §1.6. + +### 3.3 `calculateRanksCompact` is the hot loop — micro-opts *(perf)* +`pvp_core.go:188` inner triple loop is called per stat triple per level cap. Easy wins: +- Move `calculatePvPStat` allocation: `out` is already a pointer write, good. But `multiplier := calculateCpMultiplier(level)` (`pvp_core.go:34`) is called from `calculateCp` inside `calculatePvPStat`'s binary-search loop AND once for the final level — recompute fine, but cache the `multiplier*multiplier` product to skip a mul. +- `calculateCp` inside binary search: ignore the `< 10` clamp during search — the clamp only affects the very low end where the search has long since narrowed. Saves a branch per iteration. (Verify with tests first.) +- Pre-allocate the `[4096]PvPRankingStats` once in `Ohbem` (sync.Pool) instead of `new(...)` per call. With cache disabled (master league) this allocates ~16 KiB per call. +- For default comparator path, avoid `sort.Sort` interface calls (one method dispatch per `Less`/`Swap`). A monomorphic `sort.Slice` over a typed slice or hand-rolled introsort is measurably faster on 4096 items in this hot path. Even better: for the default comparator, the keys are `(value, attack)` — sort with `slices.SortFunc` (Go 1.21+) which is faster than `sort.Sort`. + +### 3.4 `roundFloat(x, 5)` — eliminate `math.Pow` *(easy)* +`utils.go:13` — `math.Pow(10, float64(precision))` for a constant 5 in every caller. Replace with a constant: +```go +const roundFactor5 = 100000.0 +func roundPercent(v float64) float64 { return math.Round(v*roundFactor5) / roundFactor5 } +``` +or specialize `roundFloat` with a switch on `precision`. The current generic version dominates `Percentage` computation cost. + +### 3.5 `containsInt` linear scan *(low)* +`utils.go:18` — fine for short slices (`CostumeOverrideEvolutions` rarely > 5). Leave as is unless profiling indicates otherwise. + +### 3.6 `calculateAllRanksCompact` cache key build *(see §2.4)* +Bit packing also wins ~3 ns per call. + +### 3.7 `QueryPvPRank` sort of `combinationIndexKeys` *(perf)* +`ohbem.go:465–471` allocates a slice and sorts to iterate `combinationIndex` in ascending level order. But the level keys come from `o.LevelCaps` (already user-ordered) plus optional `MaxLevel`. Iterate `o.LevelCaps` directly with a presence check; append `MaxLevel` if present. Avoids the per-call alloc + sort. + +### 3.8 `CalculateTopRanks` parallel slice `lastRank` / `lastRankIdx` *(cleanup)* +`ohbem.go:250–303` keeps two parallel slices growing in lock-step. Combine into one struct slice for clarity and cache locality: +```go +type lastEntry struct { ranking Ranking; idx int } +var last []lastEntry +``` + +### 3.9 Rebuild `*http.Client` per call *(low)* +`utils.go:34` — once §1.1 is in place, also share the client across calls (package var or field on `Ohbem`). + +### 3.10 `json.Marshal` of full `PokemonData` for change detection *(see §3.1)* +After fetching, you have raw bytes from `resp.Body`. Decode into `PokemonData` *and* keep the raw bytes (read into buffer first via `io.ReadAll(io.LimitReader(...))`, then `json.Unmarshal`). Compare raw incoming bytes against the previous raw bytes — no remarshal needed. + +### 3.11 `sync.Map` is not the best fit for `compactRankCache` *(consider)* +`structs.go:18` — `sync.Map` shines for write-once-read-many keys with disjoint key sets across goroutines. Here keys are derived from `(cpCap, stats)` — heavily shared. A `map[int64]map[int]compactCacheValue` guarded by `sync.RWMutex` (or sharded map) often outperforms `sync.Map` for this access pattern. Benchmark before changing — `BenchmarkCalculateAllRanksCompactCached` is your yardstick. + +--- + +## 4. Deduplication / Cleanup + +### 4.1 Repeated "lookup form / fallback to base / lookup evolution" pattern +The same triple‑lookup logic appears in `CalculateTopRanks` (lines 198–242), `CalculateCp` (357–388), `QueryPvPRank` (404–427 + 524–528), and `FindBaseStats` (584–626). Extract a helper: +```go +func (o *Ohbem) resolveStats(pokemonId, form, evolution int) (PokemonStats, Form, Pokemon, bool) { + mp, ok := o.PokemonData.Pokemon[pokemonId] + if !ok { return PokemonStats{}, Form{}, Pokemon{}, false } + mf, ok := mp.Forms[form] + if !ok || form == 0 { + mf = Form{Attack: mp.Attack, Defense: mp.Defense, Stamina: mp.Stamina, + Little: mp.Little, Evolutions: mp.Evolutions, + TempEvolutions: mp.TempEvolutions, CostumeOverrideEvolutions: mp.CostumeOverrideEvolutions} + } + var stats PokemonStats + if me, ok := mf.TempEvolutions[evolution]; ok && evolution != 0 && me.Attack != 0 { + stats = me + } else if me, ok := mp.TempEvolutions[evolution]; ok && evolution != 0 { + stats = me + } else if mf.Attack != 0 { + stats = PokemonStats{Attack: mf.Attack, Defense: mf.Defense, Stamina: mf.Stamina} + } else { + stats = PokemonStats{Attack: mp.Attack, Defense: mp.Defense, Stamina: mp.Stamina} + } + return stats, mf, mp, true +} +``` +Eliminates ~120 lines and forces a single, testable resolution rule (which would have caught §2.1 / §2.8). + +### 4.2 Commented-out `calculateRanks` / `TestCalculateRanks` *(cleanup)* +`pvp_core.go:75–113` and `pvp_core_test.go:158–238` are large commented blocks. Either delete or move to a separate experimental file. Dead code rots and confuses readers. Git history preserves it. + +### 4.3 `goland:noinspection` comment *(cleanup)* +`utils.go:39` — IDE-specific marker. Either suppress globally in a config or use Go's idiom: just `defer resp.Body.Close()` is fine; staticcheck/`errcheck` won’t flag a deferred Close in stdlib usage. + +### 4.4 Lazy `RankingComparator` default duplicated *(see §3.2)* +Two copies (`ohbem.go:149` and `:244`). + +### 4.5 `else` after `return` in `StopWatchingPokemonData` *(style)* +`ohbem.go:124–131` — `if cond { return … } else { close(...) }` simplifies to early return. + +### 4.6 `containsInt` could be `slices.Contains` *(Go 1.21+)* +`utils.go:18` — replace with stdlib `slices.Contains` once go.mod allows. + +### 4.7 Unused `Index` field on `Ranking` *(cleanup)* +`structs.go:71` — `Index int` is in the JSON shape with `omitempty` but never set anywhere in code. Either remove or document. + +### 4.8 Inconsistent error variable scope +`errors.go` mixes errors that reflect user input (`ErrQueryInputOutOfRange`, `ErrMissingPokemon`), runtime state (`ErrMasterFileUnloaded`, `ErrLeaguesMissing`), and remote IO (`ErrMasterFileFetch`). Group with comments or split into `errors_input.go` / `errors_io.go` / etc. + +--- + +## 5. Logic / API Recommendations + +### 5.1 Add `New(config)` constructor *(API)* +Currently callers build `Ohbem{...}` directly and the library has to defend against missing fields lazily on every call (`safetyCheck`, comparator default, cache init). A constructor: +```go +func New(opts Options) (*Ohbem, error) { ... } +``` +- centralizes defaults (comparator, watcher interval, cache), +- returns `*Ohbem` to make the mutex semantics obvious, +- removes need for lazy mutation in hot paths, +- enables marking unexported fields explicitly. + +### 5.2 Receivers should be pointer everywhere *(consistency)* +All receivers are `*Ohbem` already; with the cache & watcher state, callers must always pass pointers. Document this in README and in the type doc comment. + +### 5.3 Watcher API *(ergonomics)* +- Provide `WatchPokemonDataContext(ctx context.Context)` so users can cancel via context instead of a `Stop` method (Go idiom). Drop the bool channel. +- Surface fetch errors via a callback or error channel rather than a `Logger.Print` string. + +### 5.4 `Logger` should accept levels *(observability)* +`structs.go:37` — single `Print(string)` flattens info, warning, error. Either: +- Adopt `log/slog` (`*slog.Logger`), or +- Pass `(level, msg, fields...)`. + +### 5.5 Document concurrency contract +README (`README.md:55`) shows `ohbem := gohbem.Ohbem{...}` — a value, not pointer. With `sync.Map` / channel state, mixing values and pointers risks copying. Add a "Concurrency" section: must-be-pointer, methods are safe under the rules of §1.6 once the mutex is added. + +### 5.6 `CalculateTopRanks` "broken" tag in README *(action)* +`README.md:92` flags the function as broken. The recent `len-fixes` branch (`695642a fix: uncomment and fix CalculateTopRanks…`) indicates work in progress. Once the master-league bug (§2.3) and the commented capped tests (`ohbem_test.go:110–111`) are addressed, drop the "broken" tag. + +### 5.7 `DisableCache` semantics *(API)* +`structs.go:13` — `DisableCache bool`. Inverted-default flags read awkwardly. Prefer `EnableCache bool` defaulting to false, *or* make caching the default and offer a `WithoutCache()` option. + +### 5.8 MasterFile URL hardcoded *(API)* +`utils.go:11` — `MasterFileURL` is a top-level `const`. Make it overridable on `Ohbem` (e.g. `MasterFileURL string`) so users can self-host or air-gap. + +### 5.9 Error wrapping *(idiomatic)* +All errors are sentinel-only (`errors.go`). The actual underlying cause (network error, file path, JSON offset) is discarded. Wrap with `fmt.Errorf("fetch masterfile: %w", err)` and keep the sentinel via `errors.Is`. + +### 5.10 `safetyCheck` duplicates `o != nil` assumption *(defensive)* +`utils.go:51` — passing a nil `*Ohbem` panics before `safetyCheck` runs. Add `if o == nil { return ErrNotInitialized }` if `*Ohbem` is the public type. + +--- + +## 6. Test Gaps + +- `CalculateTopRanks` master league not exercised (related to §2.3). +- `FilterLevelCaps` only asserts `len(output)`, not `Cap` / `Capped` (related to §2.2). +- No `-race` run in CI (`.github/workflows/test.yml` not inspected here, but adding `go test -race ./...` will surface §1.6 immediately). +- No fuzz tests for `QueryPvPRank` IV bounds. +- No test for `WatchPokemonData` start→stop→start sequence (related to §1.7). +- No test for `FetchPokemonData` error paths (timeout, non-200, malformed JSON). + +--- + +## 7. Priority Punch‑list + +| # | Item | Severity | Effort | +|---|------|----------|--------| +| 1 | Add HTTP timeout + LimitReader + status check (§1.1–1.3) | High | S | +| 2 | Fix data races: mutex + atomic cache pointer (§1.6) | High | M | +| 3 | Fix `FindBaseStats` else clobber (§2.1) | High | XS | +| 4 | Fix `FilterLevelCaps` value-copy mutation (§2.2) | High | XS | +| 5 | Restartable watcher + nil-out channel (§1.7) | Medium | XS | +| 6 | Master-league `<= 15` loop bound (§2.3) | Medium | XS | +| 7 | Replace SHA-256 with byte compare; reuse last bytes (§3.1, §3.10) | Medium | S | +| 8 | Bit-pack cacheKey (§2.4) | Medium | XS | +| 9 | Atomic file write for SavePokemonData (§1.5) | Low | XS | +| 10 | Extract `resolveStats` helper (§4.1) | Low | M | +| 11 | Move comparator default to constructor (§3.2) | Low | S | +| 12 | Drop dead commented blocks (§4.2) | Low | XS | +| 13 | `go test -race` in CI | High | XS | +| 14 | Fuzz tests for IV/level bounds (§6) | Low | M | + +--- + +## 8. Summary + +The library is small, focused, and the hot path is well shaped — `calculateRanksCompact` shows real attention to allocation and sort cost. Two real correctness bugs (`FindBaseStats` else clobber, `FilterLevelCaps` value-copy mutate) are masked by gaps in test assertions; both are one-line fixes. The most pressing risks are the **HTTP client without timeout** and **unsynchronized shared state** between the watcher goroutine and query callers — both will bite under production load. The proposed refactor (constructor + mutex + helper resolver) will simultaneously fix the races, eliminate ~120 lines of duplication, and make further optimizations (atomic-pointer cache swap, sync.Pool for the 16 KiB rank arena) safe to apply. + +--- + +## 9. Status (post-fix pass — 2026-05-05) + +| Item | Status | Notes | +|------|--------|-------| +| 1.1 HTTP timeout | ✅ done | Package-level `httpClient` with 30s `Timeout` (`utils.go`). | +| 1.2 LimitReader 32 MiB | ✅ done | `io.LimitReader(resp.Body, 32<<20)` in `fetchMasterFile`. | +| 1.3 HTTP status check | ✅ done | Non-200 → `ErrMasterFileFetch`. | +| 1.4 LoadPokemonData OOM | ⏭ skipped | Local file is trusted-caller scope; HTTP path covered by 1.2. | +| 1.5 SavePokemonData mode/atomic | ⏭ skipped | Per request. Watcher path uses tmp+Rename anyway (see 2.5). | +| 1.6 Data races | ✅ done | `sync.RWMutex` on `Ohbem`; `compactRankCache` is `atomic.Pointer[sync.Map]`; comparator default set under lock. | +| 1.7 Restartable watcher | ✅ done | `StopWatchingPokemonData` nils channel; second stop returns `ErrNilChannel`; covered by `TestWatchPokemonDataRestart`. | +| 1.8 Mutable error vars | ⏭ informational | Convention kept. | +| 1.9 User-Agent injection | ✅ done | `VERSION` is a `const`; no runtime input flows into header. | +| 2.1 `FindBaseStats` else clobber | ✅ done | Replaced with `resolveStats` helper (no else-clobber path). | +| 2.2 `FilterLevelCaps` value copy | ✅ done | Mutations applied via `&result[len-1]`. Regression test `TestFilterLevelCapsMerge`. | +| 2.3 Master league `<= 15` | ✅ done | Loop bound updated; `CalculateTopRanks` master-league path now produces 15/15/15. | +| 2.4 Cache key bit-pack | ✅ done | `cacheKey()` packs `(cpCap, attack, defense, stamina)` into one int64. | +| 2.5 Watcher save before swap | ✅ done | Tmp-file + `os.Rename`; on save failure, in-memory swap is skipped. | +| 2.6 `calculateCpMultiplier` half-level | ⏭ skipped | Per request. | +| 2.7 Binary search midpoint | ⏭ verify only | Behavior preserved; left as is. | +| 2.8 `IsMegaUnreleased` arg name | ✅ done | Renamed second arg to `tempEvolution`; doc clarifies it is a TempEvolution key. | +| 2.9 Recursion cycle guard | ✅ done | Depth bound (`maxEvolutionDepth=8`) on `queryPvPRankInternal`. | +| 2.10 Positional struct literal | ✅ done | `resolveStats` builds `PokemonStats` with named fields throughout. | +| 2.11 `CalculateCp` IV/level validation | ✅ done | Same `ErrQueryInputOutOfRange` gate as `QueryPvPRank`. | +| 3.1 Hash → byte compare | ⏭ skipped | Per request. | +| 3.2 Comparator default in init | ✅ done | Hoisted into `LoadPokemonData` / `FetchPokemonData` under write lock; lazy fallback kept inside `calculateAllRanksCompact` for safety but no longer the primary path. | +| 3.3 Hot loop micro-opts | ✅ partial | `sync.Pool` for the 16 KiB `[4096]PvPRankingStats` arena (`releaseRankArena`); `sort.Sort` interface replaced with `slices.SortFunc`. | +| 3.4 `roundFloat(x, 5)` constant | ✅ done | Fast path with `roundFactor5` in `utils.go`. | +| 3.5 `containsInt` linear scan | ✅ done | Removed; replaced with `slices.Contains`. | +| 3.6 cacheKey perf | ✅ done | Covered by 2.4. | +| 3.7 `QueryPvPRank` sort | ✅ done | Iterates `o.LevelCaps` directly + optional `MaxLevel`; no per-call alloc/sort. | +| 3.8 Parallel slice cleanup | ✅ done | Replaced `lastRank` / `lastRankIdx` with one `[]lastEntry`. | +| 3.9 Reuse `*http.Client` | ✅ done | Package-level `httpClient`. | +| 3.10 Keep raw bytes | ⏭ skipped | Tied to 3.1. | +| 3.11 sync.Map vs RWMutex map | ⏭ deferred | Needs benchmark; cache is now `atomic.Pointer[sync.Map]` so this can be revisited safely. | +| 4.1 `resolveStats` helper | ✅ done | New helper in `ohbem.go`; used by `CalculateTopRanks`, `CalculateCp`, `QueryPvPRank` (`queryPvPRankInternal`), `FindBaseStats`. | +| 4.2 Dead commented blocks | ✅ done | Removed from `pvp_core.go` and `pvp_core_test.go`. | +| 4.3 `goland:noinspection` | ✅ done | Removed; deferred Close kept. | +| 4.4 Comparator default duplicate | ✅ done | Centralized via 3.2. | +| 4.5 `else` after return in Stop | ✅ done | Early-return form. | +| 4.6 `slices.Contains` | ✅ done | All call sites. | +| 4.7 Unused `Index` on `Ranking` | ✅ done | Field removed. | +| 4.8 Errors regrouped | ✅ done | Grouped into Input / Runtime / I/O / Internal blocks in `errors.go`. | +| 5.6 Drop "broken" tag | ✅ done | README heading updated. | +| 5.8 MasterFileURL configurable | ✅ done | New `Ohbem.MasterFileURL` field; `fetchMasterFile(url)` accepts override; default fallback preserved; documented in README. | +| 6.* Test gaps | ✅ done | Master-league `TestCalculateTopRanks` cases re-enabled; `TestFilterLevelCapsMerge` asserts Cap+Capped propagation; `TestWatchPokemonDataRestart` covers start→stop→start + double-stop; `TestFetchPokemonDataNon200` / `TestFetchPokemonDataMalformed` cover error paths via `httptest`; `TestSavePokemonDataAtomic` round-trips a save; `FuzzQueryPvPRankBounds` and `FuzzCalculateCpBounds` cover IV/level bounds; CI workflow now runs `go test -race`. | + +### Verification + +- `go build ./...` clean. +- `go vet ./...` clean. +- `go test -race ./...` passes (`ok github.com/UnownHash/gohbem`). +- VERSION bumped to `0.13.0` to reflect API additions (`Ohbem.MasterFileURL`, removal of `Ranking.Index`, `IsMegaUnreleased` arg rename). + +### Items intentionally left + +- 1.4 (file size cap), 1.5 (Save mode/atomic), 1.8 (mutable error vars), 2.6 / 2.7 (CPM / midpoint deep-dive), 3.1 / 3.10 (raw-byte change detection), 3.11 (sync.Map alternative — needs benchmark), 5.x items not requested (constructor, context-based watcher, Logger levels, error wrapping, `DisableCache` rename, etc.). diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..1158111 --- /dev/null +++ b/Makefile @@ -0,0 +1,51 @@ +.PHONY: all build test race vet lint bench fuzz-query fuzz-cp tidy clean help + +GO ?= go +PKG ?= ./... +FUZZTIME ?= 30s + +all: vet test + +build: + $(GO) build $(PKG) + +test: + $(GO) test $(PKG) + +race: + $(GO) test -race $(PKG) + +vet: + $(GO) vet $(PKG) + +lint: + golangci-lint run + +bench: + $(GO) test -bench=. -benchmem -run=^$$ $(PKG) + +fuzz-query: + $(GO) test -run=^$$ -fuzz=FuzzQueryPvPRankBounds -fuzztime=$(FUZZTIME) . + +fuzz-cp: + $(GO) test -run=^$$ -fuzz=FuzzCalculateCpBounds -fuzztime=$(FUZZTIME) . + +tidy: + $(GO) mod tidy + +clean: + $(GO) clean -testcache + rm -rf testdata/fuzz + +help: + @echo "Targets:" + @echo " build compile package" + @echo " test run unit tests" + @echo " race run unit tests with -race" + @echo " vet go vet" + @echo " lint golangci-lint run" + @echo " bench run benchmarks" + @echo " fuzz-query fuzz QueryPvPRank bounds (FUZZTIME=$(FUZZTIME))" + @echo " fuzz-cp fuzz CalculateCp bounds (FUZZTIME=$(FUZZTIME))" + @echo " tidy go mod tidy" + @echo " clean drop test cache + fuzz corpus" diff --git a/README.md b/README.md index bd84c1c..d078aac 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,8 @@ func main() { levelCaps := []int{50, 51} // Level caps. ohbem := gohbem.Ohbem{Leagues: leagues, LevelCaps: levelCaps} + // Optional: override the upstream MasterFile location (self-hosted / air-gapped). + // ohbem.MasterFileURL = "https://example.com/master-latest-basics.json" err = ohbem.FetchPokemonData() // Fetch latest stable MasterFile... err = ohbem.WatchPokemonData() // ...automatically watch remote for changes... @@ -89,7 +91,7 @@ entries, err := ohbem.QueryPvPRank(605, 0, 0, 1, 1, 4, 12, 7) } ``` -### CalculateTopRanks (broken) +### CalculateTopRanks ```go entries, err := ohbem.CalculateTopRanks(5, 605, 0, 0, 0) diff --git a/errors.go b/errors.go index d8e43ba..c6d95d5 100644 --- a/errors.go +++ b/errors.go @@ -2,44 +2,56 @@ package gohbem import "errors" -// ErrNilChannel is returned when o.watcherChan is uninitialized. -var ErrNilChannel = errors.New("can't close nil channel") +// User input errors — caller passed something invalid. +var ( + // ErrQueryInputOutOfRange is returned when wrong arguments are passed to QueryPvPRank function. + ErrQueryInputOutOfRange = errors.New("one of input arguments 'Attack, Defense, Stamina, Level' is out of range") -// ErrMasterFileUnloaded is returned when MasterFile wasn't loaded but there was a need to use it. -var ErrMasterFileUnloaded = errors.New("masterFile unloaded") + // ErrMissingPokemon is returned when Pokemon is missing in MasterFile. + ErrMissingPokemon = errors.New("missing pokemonID in MasterFile") +) -// ErrMasterFileOpen is returned when MasterFile can't be open. -var ErrMasterFileOpen = errors.New("can't open MasterFile") +// Runtime state errors — Ohbem instance is not in a usable state. +var ( + // ErrMasterFileUnloaded is returned when MasterFile wasn't loaded but there was a need to use it. + ErrMasterFileUnloaded = errors.New("masterFile unloaded") -// ErrMasterFileSave is returned when MasterFile can't be saved. -var ErrMasterFileSave = errors.New("can't save MasterFile") + // ErrLeaguesMissing is returned when Leagues configuration is empty. + ErrLeaguesMissing = errors.New("leagues configuration is empty") -// ErrMasterFileMarshall is returned when Marshal of MasterFile fail. -var ErrMasterFileMarshall = errors.New("can't marshal MasterFile") + // ErrLevelCapsMissing is returned when levelCaps configuration is empty. + ErrLevelCapsMissing = errors.New("levelCaps configuration is empty") -// ErrMasterFileUnmarshall is returned when UnMarshal of MasterFile fail. -var ErrMasterFileUnmarshall = errors.New("can't unmarshal MasterFile") + // ErrNilChannel is returned when o.watcherChan is uninitialized. + ErrNilChannel = errors.New("can't close nil channel") -// ErrMasterFileFetch is returned when remote fetch of MasterFile fail. -var ErrMasterFileFetch = errors.New("can't fetch remote MasterFile") + // ErrWatcherStarted is returned when MasterFile Watcher is already running. + ErrWatcherStarted = errors.New("MasterFile Watcher Already Started") +) -// ErrMasterFileDecode is returned when decode of MasterFile fail. -var ErrMasterFileDecode = errors.New("can't decode remote MasterFile") +// I/O errors — fetching, loading, or saving the MasterFile failed. +var ( + // ErrMasterFileOpen is returned when MasterFile can't be open. + ErrMasterFileOpen = errors.New("can't open MasterFile") -// ErrWatcherStarted is returned when MasterFile Watcher is already running. -var ErrWatcherStarted = errors.New("MasterFile Watcher Already Started") + // ErrMasterFileSave is returned when MasterFile can't be saved. + ErrMasterFileSave = errors.New("can't save MasterFile") -// ErrQueryInputOutOfRange is returned when wrong arguments are passed to QueryPvPRank function. -var ErrQueryInputOutOfRange = errors.New("one of input arguments 'Attack, Defense, Stamina, Level' is out of range") + // ErrMasterFileMarshall is returned when Marshal of MasterFile fail. + ErrMasterFileMarshall = errors.New("can't marshal MasterFile") -// ErrMissingPokemon is returned when Pokemon is missing in MasterFile. -var ErrMissingPokemon = errors.New("missing pokemonID in MasterFile") + // ErrMasterFileUnmarshall is returned when UnMarshal of MasterFile fail. + ErrMasterFileUnmarshall = errors.New("can't unmarshal MasterFile") -// ErrPvpStatBestCp is returned when BestCP > Cap in calculatePvPStat function. -var ErrPvpStatBestCp = errors.New("bestCP > cap") + // ErrMasterFileFetch is returned when remote fetch of MasterFile fail. + ErrMasterFileFetch = errors.New("can't fetch remote MasterFile") -// ErrLeaguesMissing is returned when Leagues configuration is empty. -var ErrLeaguesMissing = errors.New("leagues configuration is empty") + // ErrMasterFileDecode is returned when decode of MasterFile fail. + ErrMasterFileDecode = errors.New("can't decode remote MasterFile") +) -// ErrLevelCapsMissing is returned when levelCaps configuration is empty. -var ErrLevelCapsMissing = errors.New("levelCaps configuration is empty") +// Internal computation errors. +var ( + // ErrPvpStatBestCp is returned when BestCP > Cap in calculatePvPStat function. + ErrPvpStatBestCp = errors.New("bestCP > cap") +) diff --git a/ohbem.go b/ohbem.go index 33aa272..29bd445 100644 --- a/ohbem.go +++ b/ohbem.go @@ -1,12 +1,12 @@ package gohbem import ( - "crypto/sha256" + "bytes" "encoding/json" "fmt" "math" "os" - "sort" + "slices" "sync" "time" ) @@ -15,16 +15,20 @@ import ( const MaxLevel = 100 // VERSION of gohbem, follows Semantic Versioning. (http://semver.org/) -const VERSION = "0.12.0" +const VERSION = "0.13.0" // FetchPokemonData Fetch remote MasterFile and keep it in memory. func (o *Ohbem) FetchPokemonData() error { - var err error - - o.PokemonData, err = fetchMasterFile() + data, err := fetchMasterFile(o.MasterFileURL) if err != nil { return err } + o.mu.Lock() + o.PokemonData = data + if o.RankingComparator == nil { + o.RankingComparator = RankingComparatorDefault + } + o.mu.Unlock() o.ClearCache() return nil } @@ -35,17 +39,26 @@ func (o *Ohbem) LoadPokemonData(filePath string) error { if err != nil { return ErrMasterFileOpen } - if err := json.Unmarshal(data, &o.PokemonData); err != nil { + var pd PokemonData + if err := json.Unmarshal(data, &pd); err != nil { return ErrMasterFileUnmarshall } - o.PokemonData.Initialized = true + pd.Initialized = true + o.mu.Lock() + o.PokemonData = pd + if o.RankingComparator == nil { + o.RankingComparator = RankingComparatorDefault + } + o.mu.Unlock() o.ClearCache() return nil } // SavePokemonData Save MasterFile from memory to provided location. func (o *Ohbem) SavePokemonData(filePath string) error { + o.mu.RLock() data, err := json.Marshal(o.PokemonData) + o.mu.RUnlock() if err != nil { return ErrMasterFileMarshall } @@ -57,63 +70,71 @@ func (o *Ohbem) SavePokemonData(filePath string) error { // WatchPokemonData Watch for remote MasterFile changes. When new, auto-update and clean cache. func (o *Ohbem) WatchPokemonData() error { + o.mu.Lock() if o.watcherChan != nil { + o.mu.Unlock() return ErrWatcherStarted } - - o.log("MasterFile Watcher Started") o.watcherChan = make(chan bool) - var interval time.Duration + stopCh := o.watcherChan + o.mu.Unlock() - // if interval is not provided, use 60 minutes - if o.WatcherInterval == 0 { + o.log("MasterFile Watcher Started") + interval := o.WatcherInterval + if interval == 0 { interval = 60 * time.Minute - } else { - interval = o.WatcherInterval } go func() { ticker := time.NewTicker(interval) + defer ticker.Stop() for { select { - case <-o.watcherChan: + case <-stopCh: o.log("MasterFile Watcher Stopped") - ticker.Stop() return case <-ticker.C: o.log("Checking remote MasterFile") - pokemonData, err := fetchMasterFile() + pokemonData, err := fetchMasterFile(o.MasterFileURL) if err != nil { o.log("Remote MasterFile fetch failed") continue } - newData, hashErr := json.Marshal(pokemonData) - if hashErr != nil { - o.log("Remote MasterFile hash failed") + newData, mErr := json.Marshal(pokemonData) + if mErr != nil { + o.log("Remote MasterFile marshal failed") continue } - oldData, hashErr := json.Marshal(o.PokemonData) - if hashErr != nil { - o.log("Current MasterFile hash failed") + o.mu.RLock() + oldData, mErr := json.Marshal(o.PokemonData) + o.mu.RUnlock() + if mErr != nil { + o.log("Current MasterFile marshal failed") continue } - if sha256.Sum256(newData) == sha256.Sum256(oldData) { + if bytes.Equal(newData, oldData) { continue - } else { - o.log("New MasterFile found! Updating PokemonData") - o.PokemonData = pokemonData // overwrite PokemonData using new MasterFile - o.PokemonData.Initialized = true - o.ClearCache() // clean compactRankCache cache - // when provided store latest version of MasterFile under provided path - if o.MasterFileCachePath != "" { - err = o.SavePokemonData(o.MasterFileCachePath) - if err != nil { - o.log(fmt.Sprintf("Storing MasterFile cache under %s has failed!", o.MasterFileCachePath)) - continue - } + } + o.log("New MasterFile found! Updating PokemonData") + // Save first to disk; only swap in-memory if save succeeded so a crash + // before swap doesn't leave cache file pointing at the old version while + // the running process serves the new one. + if o.MasterFileCachePath != "" { + tmp := o.MasterFileCachePath + ".tmp" + if err := os.WriteFile(tmp, newData, 0644); err != nil { + o.log(fmt.Sprintf("Storing MasterFile cache under %s has failed!", o.MasterFileCachePath)) + continue + } + if err := os.Rename(tmp, o.MasterFileCachePath); err != nil { + o.log(fmt.Sprintf("Renaming MasterFile cache to %s has failed!", o.MasterFileCachePath)) + continue } } + o.mu.Lock() + o.PokemonData = pokemonData + o.mu.Unlock() + o.ClearCache() } } }() @@ -122,32 +143,60 @@ func (o *Ohbem) WatchPokemonData() error { // StopWatchingPokemonData Stop watching for remote MasterFile changes. func (o *Ohbem) StopWatchingPokemonData() error { + o.mu.Lock() + defer o.mu.Unlock() if o.watcherChan == nil { return ErrNilChannel - } else { - close(o.watcherChan) } + close(o.watcherChan) + o.watcherChan = nil return nil } +// ClearCache empties the compact rank cache by atomically swapping in a fresh map. func (o *Ohbem) ClearCache() { if !o.DisableCache { - o.compactRankCache = sync.Map{} + o.compactRankCache.Store(&sync.Map{}) o.log("Cache cleaned") } } +// cacheKey packs (cpCap, attack, defense, stamina) into one int64. +// Stats fit in 16 bits and cpCap in <= 14 bits, so no overflow / collisions. +func cacheKey(cpCap int, stats *PokemonStats) int64 { + return int64(cpCap)<<48 | int64(stats.Attack)<<32 | int64(stats.Defense)<<16 | int64(stats.Stamina) +} + +// loadCache returns the current cache map, allocating one lazily if needed. +func (o *Ohbem) loadCache() *sync.Map { + m := o.compactRankCache.Load() + if m == nil { + fresh := &sync.Map{} + if o.compactRankCache.CompareAndSwap(nil, fresh) { + return fresh + } + return o.compactRankCache.Load() + } + return m +} + // calculateAllRanksCompact Calculate all PvP ranks for a specific base stats with the specified CP cap. Compact version intended to be used with cache. func (o *Ohbem) calculateAllRanksCompact(stats *PokemonStats, cpCap int) (map[int]compactCacheValue, bool) { - cacheKey := int64(cpCap*999*999*999 + stats.Attack*999*999 + stats.Defense*999 + stats.Stamina) + key := cacheKey(cpCap, stats) + var cache *sync.Map if !o.DisableCache { - if obj, ok := o.compactRankCache.Load(cacheKey); ok { + cache = o.loadCache() + if obj, ok := cache.Load(key); ok { return obj.(map[int]compactCacheValue), true } } - if o.RankingComparator == nil { - o.RankingComparator = RankingComparatorDefault + + o.mu.RLock() + comparator := o.RankingComparator + o.mu.RUnlock() + if comparator == nil { + comparator = RankingComparatorDefault } filled := false @@ -160,12 +209,12 @@ func (o *Ohbem) calculateAllRanksCompact(stats *PokemonStats, cpCap int) (map[in continue } - combinations, sortedRanks := calculateRanksCompact(stats, cpCap, lvCapFloat, o.RankingComparator, 0) - res := compactCacheValue{ + combinations, sortedRanks := calculateRanksCompact(stats, cpCap, lvCapFloat, comparator, 0) + result[lvCap] = compactCacheValue{ Combinations: combinations, TopValue: sortedRanks[0].Value, } - result[lvCap] = res + releaseRankArena(sortedRanks) filled = true if calculateCp(stats, 0, 0, 0, lvCapFloat+0.5) > cpCap { maxed = true @@ -173,20 +222,58 @@ func (o *Ohbem) calculateAllRanksCompact(stats *PokemonStats, cpCap int) (map[in } } if filled && !maxed { - combinations, sortedRanks := calculateRanksCompact(stats, cpCap, MaxLevel, o.RankingComparator, 0) - - res := compactCacheValue{ + combinations, sortedRanks := calculateRanksCompact(stats, cpCap, MaxLevel, comparator, 0) + result[MaxLevel] = compactCacheValue{ Combinations: combinations, TopValue: sortedRanks[0].Value, } - result[MaxLevel] = res + releaseRankArena(sortedRanks) } if !o.DisableCache && filled { - o.compactRankCache.Store(cacheKey, result) + cache.Store(key, result) } return result, filled } +// resolveStats returns the stats, form, pokemon, and existence for a (pokemon, form, evolution) tuple. +// Lookup order for stats: form's TempEvolution, pokemon's TempEvolution, form base, pokemon base. +// Caller must already hold o.mu (read lock) when accessing PokemonData fields. +func resolveStats(pd *PokemonData, pokemonId, form, evolution int) (PokemonStats, Form, Pokemon, bool) { + mp, ok := pd.Pokemon[pokemonId] + if !ok { + return PokemonStats{}, Form{}, Pokemon{}, false + } + mf, hasForm := mp.Forms[form] + if !hasForm || form == 0 { + mf = Form{ + Attack: mp.Attack, + Defense: mp.Defense, + Stamina: mp.Stamina, + Little: mp.Little, + Evolutions: mp.Evolutions, + TempEvolutions: mp.TempEvolutions, + CostumeOverrideEvolutions: mp.CostumeOverrideEvolutions, + } + } + var stats PokemonStats + if evolution != 0 { + if me, ok := mf.TempEvolutions[evolution]; ok && me.Attack != 0 { + stats = me + return stats, mf, mp, true + } + if me, ok := mp.TempEvolutions[evolution]; ok && me.Attack != 0 { + stats = me + return stats, mf, mp, true + } + } + if mf.Attack != 0 { + stats = PokemonStats{Attack: mf.Attack, Defense: mf.Defense, Stamina: mf.Stamina} + } else { + stats = PokemonStats{Attack: mp.Attack, Defense: mp.Defense, Stamina: mp.Stamina} + } + return stats, mf, mp, true +} + // CalculateTopRanks Return ranked list of PVP statistics for a given Pokémon. func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolution int, ivFloor int) (map[string][]Ranking, error) { result := make(map[string][]Ranking) @@ -195,74 +282,39 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut return result, err } - masterPokemon := o.PokemonData.Pokemon[pokemonId] - - if masterPokemon.Attack == 0 { + o.mu.RLock() + stats, masterForm, masterPokemon, ok := resolveStats(&o.PokemonData, pokemonId, form, evolution) + comparator := o.RankingComparator + o.mu.RUnlock() + if !ok || masterPokemon.Attack == 0 { return result, nil } - - var masterForm Form - if _, ok := masterPokemon.Forms[form]; ok && form != 0 { - masterForm = masterPokemon.Forms[form] - } else { - masterForm = Form{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - Little: masterPokemon.Little, - } - } - - var masterEvolution PokemonStats - if _, ok := masterForm.TempEvolutions[evolution]; ok && evolution != 0 { - masterEvolution = masterForm.TempEvolutions[evolution] - } else { - masterEvolution = PokemonStats{ - Attack: masterForm.Attack, - Defense: masterForm.Defense, - Stamina: masterForm.Stamina, - } - } - - var stats PokemonStats - if masterEvolution.Attack != 0 { - stats = masterEvolution - } else if masterForm.Attack != 0 { - stats = PokemonStats{ - Attack: masterForm.Attack, - Defense: masterForm.Defense, - Stamina: masterForm.Stamina, - } - } else { - stats = PokemonStats{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - } + if comparator == nil { + comparator = RankingComparatorDefault } - if o.RankingComparator == nil { - o.RankingComparator = RankingComparatorDefault + type lastEntry struct { + ranking Ranking + idx int } for leagueName, leagueOptions := range o.Leagues { var rankings []Ranking - lastRank := make([]Ranking, 0) - lastRankIdx := make([]int, 0) // indices into rankings slice for O(1) Capped updates + var last []lastEntry processLevelCap := func(lvCap float64, setOnDup bool) { - combinations, sortedRanks := calculateRanksCompact(&stats, leagueOptions.Cap, lvCap, o.RankingComparator, ivFloor) + combinations, sortedRanks := calculateRanksCompact(&stats, leagueOptions.Cap, lvCap, comparator, ivFloor) + defer releaseRankArena(sortedRanks) - for i := 0; i < 4096; i++ { + for i := range 4096 { stat := &sortedRanks[i] if stat.Value == 0 { break } rank := combinations[stat.Index] if rank > maxRank { - for len(lastRank) > i { - lastRank = lastRank[:len(lastRank)-1] - lastRankIdx = lastRankIdx[:len(lastRankIdx)-1] + if len(last) > i { + last = last[:i] } break } @@ -271,15 +323,15 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut stamina := stat.Index % 16 var lastStat *Ranking - if i < len(lastRank) { - lastStat = &lastRank[i] + if i < len(last) { + lastStat = &last[i].ranking } if lastStat != nil && stat.Level == lastStat.Level && rank == lastStat.Rank && attack == lastStat.Attack && defense == lastStat.Defense && stamina == lastStat.Stamina { if setOnDup { - rankings[lastRankIdx[i]].Capped = true + rankings[last[i].idx].Capped = true } } else if !setOnDup { entry := Ranking{ @@ -295,12 +347,10 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut } rankingsIdx := len(rankings) rankings = append(rankings, entry) - for len(lastRank) <= i { - lastRank = append(lastRank, Ranking{}) - lastRankIdx = append(lastRankIdx, 0) + for len(last) <= i { + last = append(last, lastEntry{}) } - lastRank[i] = entry - lastRankIdx[i] = rankingsIdx + last[i] = lastEntry{ranking: entry, idx: rankingsIdx} } } } @@ -311,7 +361,7 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut for _, lvCap := range o.LevelCaps { lvCapFloat := float64(lvCap) maxHp := calculateHp(&stats, 15, lvCapFloat) - for stamina := ivFloor; stamina < 15; stamina++ { + for stamina := ivFloor; stamina <= 15; stamina++ { if calculateHp(&stats, stamina, lvCapFloat) == maxHp { entry := Ranking{ Attack: 15, @@ -335,8 +385,8 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut processLevelCap(lvCapFloat, false) if calculateCp(&stats, ivFloor, ivFloor, ivFloor, lvCapFloat+0.5) > leagueOptions.Cap { maxed = true - for _, idx := range lastRankIdx { - rankings[idx].Capped = true + for _, le := range last { + rankings[le.idx].Capped = true } break } @@ -355,78 +405,48 @@ func (o *Ohbem) CalculateTopRanks(maxRank int16, pokemonId int, form int, evolut // CalculateCp calculates CP for your pokemon. Errors if pokemon cannot be found in master. func (o *Ohbem) CalculateCp(pokemonId, form, evolution, attack, defense, stamina int, level float64) (int, error) { - masterPokemon, ok := o.PokemonData.Pokemon[pokemonId] + if (attack < 0 || attack > 15) || (defense < 0 || defense > 15) || (stamina < 0 || stamina > 15) || level < 1 { + return 0, ErrQueryInputOutOfRange + } + o.mu.RLock() + stats, _, _, ok := resolveStats(&o.PokemonData, pokemonId, form, evolution) + o.mu.RUnlock() if !ok { return 0, ErrMissingPokemon } - masterForm, ok := masterPokemon.Forms[form] - if !ok || form == 0 { - masterForm = Form{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - TempEvolutions: masterPokemon.TempEvolutions, - } - } - masterEvo, ok := masterForm.TempEvolutions[evolution] - var stats PokemonStats - if evolution != 0 && ok { - if masterEvo.Attack == 0 { - masterEvo = masterPokemon.TempEvolutions[evolution] - } - stats.Attack = masterEvo.Attack - stats.Defense = masterEvo.Defense - stats.Stamina = masterEvo.Stamina - } else if masterForm.Attack != 0 { - stats.Attack = masterForm.Attack - stats.Defense = masterForm.Defense - stats.Stamina = masterForm.Stamina - } else { - stats.Attack = masterPokemon.Attack - stats.Defense = masterPokemon.Defense - stats.Stamina = masterPokemon.Stamina - } return calculateCp(&stats, attack, defense, stamina, level), nil } -// QueryPvPRank Query all ranks for a specific Pokémon, including its possible evolutions. -func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, attack int, defense int, stamina int, level float64) (map[string][]PokemonEntry, error) { +// maxEvolutionDepth caps recursion in QueryPvPRank to guard against +// malformed (cyclic) MasterFile data. Real evolution chains are <= 3. +const maxEvolutionDepth = 8 + +// queryPvPRankInternal walks the evolution graph with a depth bound to avoid +// stack overflow on malformed (cyclic) MasterFile data. +func (o *Ohbem) queryPvPRankInternal(depth int, pokemonId, form, costume, gender, attack, defense, stamina int, level float64) (map[string][]PokemonEntry, error) { result := make(map[string][]PokemonEntry) - if err := safetyCheck(o); err != nil { - return result, err + if depth > maxEvolutionDepth { + return result, nil } - if (attack < 0 || attack > 15) || (defense < 0 || defense > 15) || (stamina < 0 || stamina > 15) || level < 1 { - return result, ErrQueryInputOutOfRange + o.mu.RLock() + stats, masterForm, masterPokemon, ok := resolveStats(&o.PokemonData, pokemonId, form, 0) + costumeBlock := false + if costume != 0 { + costumeBlock = o.PokemonData.Costumes[costume] && !slices.Contains(masterForm.CostumeOverrideEvolutions, costume) } - - var masterForm Form - var masterPokemon Pokemon - var baseEntry = PokemonEntry{Pokemon: pokemonId} - - if _, ok := o.PokemonData.Pokemon[pokemonId]; ok { - masterPokemon = o.PokemonData.Pokemon[pokemonId] - } else { + o.mu.RUnlock() + if !ok { return result, ErrMissingPokemon } - if _, ok := masterPokemon.Forms[form]; ok && form != 0 { + var baseEntry = PokemonEntry{Pokemon: pokemonId} + if _, hasForm := masterPokemon.Forms[form]; hasForm && form != 0 { baseEntry.Form = form - masterForm = masterPokemon.Forms[form] - } else { - masterForm = Form{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - Little: masterPokemon.Little, - Evolutions: masterPokemon.Evolutions, - TempEvolutions: masterPokemon.TempEvolutions, - CostumeOverrideEvolutions: masterPokemon.CostumeOverrideEvolutions, - } } - pushAllEntries := func(stats *PokemonStats, evolution int) { + pushAllEntries := func(s *PokemonStats, evolution int) { for leagueName, leagueOptions := range o.Leagues { var entries []PokemonEntry @@ -434,14 +454,14 @@ func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, a if leagueOptions.LittleCupRules && !(masterForm.Little || masterPokemon.Little) { continue } - combinationIndex, filled := o.calculateAllRanksCompact(stats, leagueOptions.Cap) + combinationIndex, filled := o.calculateAllRanksCompact(s, leagueOptions.Cap) if !filled { continue } processCombinations := func(pCap float64, combinations compactCacheValue) { var stat PvPRankingStats - if err := calculatePvPStat(&stat, stats, attack, defense, stamina, leagueOptions.Cap, pCap, level); err != nil { + if err := calculatePvPStat(&stat, s, attack, defense, stamina, leagueOptions.Cap, pCap, level); err != nil { return } entry := PokemonEntry{ @@ -461,16 +481,15 @@ func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, a entries = append(entries, entry) } - // Iterate over all combinations by sorted keys - combinationIndexKeys := make([]int, len(combinationIndex)) - indexKeysCounter := 0 - for key := range combinationIndex { - combinationIndexKeys[indexKeysCounter] = key - indexKeysCounter++ + // Iterate caps in ascending order using o.LevelCaps directly, + // then the optional MaxLevel rollup. Avoids a per-call sort+alloc. + for _, lvCap := range o.LevelCaps { + if c, ok := combinationIndex[lvCap]; ok { + processCombinations(float64(lvCap), c) + } } - sort.Ints(combinationIndexKeys) // asc order - for _, lvCap := range combinationIndexKeys { - processCombinations(float64(lvCap), combinationIndex[lvCap]) + if c, ok := combinationIndex[MaxLevel]; ok { + processCombinations(float64(MaxLevel), c) } if len(entries) == 0 { @@ -496,7 +515,7 @@ func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, a } else if evolution == 0 && attack == 15 && defense == 15 && stamina < 15 { for _, lvCap := range o.LevelCaps { lvCapFloat := float64(lvCap) - if calculateHp(stats, stamina, lvCapFloat) == calculateHp(stats, 15, lvCapFloat) { + if calculateHp(s, stamina, lvCapFloat) == calculateHp(s, 15, lvCapFloat) { entry := PokemonEntry{ Pokemon: baseEntry.Pokemon, Form: baseEntry.Form, @@ -521,16 +540,10 @@ func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, a } } - if masterForm.Attack != 0 { - pushAllEntries(&PokemonStats{masterForm.Attack, masterForm.Defense, masterForm.Stamina, false}, 0) - } else { - pushAllEntries(&PokemonStats{masterPokemon.Attack, masterPokemon.Defense, masterPokemon.Stamina, false}, 0) - } + baseStats := stats + pushAllEntries(&baseStats, 0) - canEvolve := true - if costume != 0 { - canEvolve = !o.PokemonData.Costumes[costume] || containsInt(masterForm.CostumeOverrideEvolutions, costume) - } + canEvolve := !costumeBlock if canEvolve && len(masterForm.Evolutions) != 0 { for _, evolution := range masterForm.Evolutions { switch evolution.Pokemon { @@ -550,7 +563,7 @@ func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, a if evolution.GenderRequirement != 0 && gender != evolution.GenderRequirement { continue } - evolvedRanks, _ := o.QueryPvPRank(evolution.Pokemon, evolution.Form, costume, gender, attack, defense, stamina, level) + evolvedRanks, _ := o.queryPvPRankInternal(depth+1, evolution.Pokemon, evolution.Form, costume, gender, attack, defense, stamina, level) for leagueName, results := range evolvedRanks { if result[leagueName] == nil { result[leagueName] = results @@ -563,78 +576,53 @@ func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, a if len(masterForm.TempEvolutions) != 0 { for tempEvoId, tempEvo := range masterForm.TempEvolutions { - if tempEvo.Attack != 0 { - pushAllEntries(&tempEvo, tempEvoId) - } else { - t := masterPokemon.TempEvolutions[tempEvoId] - pushAllEntries(&t, tempEvoId) + t := tempEvo + if t.Attack == 0 { + t = masterPokemon.TempEvolutions[tempEvoId] } + pushAllEntries(&t, tempEvoId) } } return result, nil } +// QueryPvPRank Query all ranks for a specific Pokémon, including its possible evolutions. +func (o *Ohbem) QueryPvPRank(pokemonId int, form int, costume int, gender int, attack int, defense int, stamina int, level float64) (map[string][]PokemonEntry, error) { + if err := safetyCheck(o); err != nil { + return make(map[string][]PokemonEntry), err + } + if (attack < 0 || attack > 15) || (defense < 0 || defense > 15) || (stamina < 0 || stamina > 15) || level < 1 { + return make(map[string][]PokemonEntry), ErrQueryInputOutOfRange + } + return o.queryPvPRankInternal(0, pokemonId, form, costume, gender, attack, defense, stamina, level) +} + // FindBaseStats Look up base stats of a Pokémon. func (o *Ohbem) FindBaseStats(pokemonId int, form int, evolution int) (PokemonStats, error) { if err := safetyCheck(o); err != nil { return PokemonStats{}, err } - - masterPokemon, ok := o.PokemonData.Pokemon[pokemonId] + o.mu.RLock() + stats, _, _, ok := resolveStats(&o.PokemonData, pokemonId, form, evolution) + o.mu.RUnlock() if !ok { return PokemonStats{}, ErrMissingPokemon } - - var masterForm Form - var masterEvolution PokemonStats - - if _, ok := masterPokemon.Forms[form]; ok && form != 0 { - masterForm = masterPokemon.Forms[form] - } else { - masterForm = Form{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - } - } - - if _, ok := masterPokemon.TempEvolutions[evolution]; ok && evolution != 0 { - masterEvolution = masterPokemon.TempEvolutions[evolution] - } else { - masterForm = Form{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - } - } - - if masterEvolution.Attack != 0 { - return masterEvolution, nil - } else if masterForm.Attack != 0 { - return PokemonStats{ - Attack: masterForm.Attack, - Defense: masterForm.Defense, - Stamina: masterForm.Stamina, - }, nil - } else { - return PokemonStats{ - Attack: masterPokemon.Attack, - Defense: masterPokemon.Defense, - Stamina: masterPokemon.Stamina, - }, nil - } + return stats, nil } // IsMegaUnreleased Check whether the stats for a given mega is speculated. -func (o *Ohbem) IsMegaUnreleased(pokemonId int, evolution int) (bool, error) { +// Second arg is a TempEvolution key (e.g. 1=MegaX, 2=MegaY), not a Form ID. +func (o *Ohbem) IsMegaUnreleased(pokemonId int, tempEvolution int) (bool, error) { if err := safetyCheck(o); err != nil { return false, err } - + o.mu.RLock() + defer o.mu.RUnlock() masterPokemon := o.PokemonData.Pokemon[pokemonId] if masterPokemon.Attack != 0 { - evo := masterPokemon.TempEvolutions[evolution] + evo := masterPokemon.TempEvolutions[tempEvolution] return evo.Unreleased, nil } return false, nil @@ -643,31 +631,32 @@ func (o *Ohbem) IsMegaUnreleased(pokemonId int, evolution int) (bool, error) { // FilterLevelCaps Filter the output of queryPvPRank with a subset of interested level caps. func (o *Ohbem) FilterLevelCaps(entries []PokemonEntry, interestedLevelCaps []int) []PokemonEntry { var result []PokemonEntry - var last PokemonEntry for _, entry := range entries { if entry.Cap == 0 { // functionally perfect, fast route for _, interested := range interestedLevelCaps { - interestedFloat := float64(interested) - if interestedFloat == entry.Level { + if float64(interested) == entry.Level { result = append(result, entry) break } } continue } - if (entry.Capped && interestedLevelCaps[len(interestedLevelCaps)-1] < int(entry.Cap)) || (!entry.Capped && !containsInt(interestedLevelCaps, int(entry.Cap))) { + if (entry.Capped && interestedLevelCaps[len(interestedLevelCaps)-1] < int(entry.Cap)) || (!entry.Capped && !slices.Contains(interestedLevelCaps, int(entry.Cap))) { continue } - if last.Pokemon != 0 && last.Pokemon == entry.Pokemon && last.Form == entry.Form && last.Evolution == entry.Evolution && last.Level == entry.Level && last.Rank == entry.Rank { - last.Cap = entry.Cap - if entry.Capped { - last.Capped = true + if len(result) > 0 { + ref := &result[len(result)-1] + if ref.Pokemon != 0 && ref.Pokemon == entry.Pokemon && ref.Form == entry.Form && ref.Evolution == entry.Evolution && ref.Level == entry.Level && ref.Rank == entry.Rank { + ref.Cap = entry.Cap + if entry.Capped { + ref.Capped = true + } + continue } - } else { - result = append(result, entry) - last = result[len(result)-1] } + result = append(result, entry) } return result } + diff --git a/ohbem_test.go b/ohbem_test.go index db620b6..4036496 100644 --- a/ohbem_test.go +++ b/ohbem_test.go @@ -2,7 +2,12 @@ package gohbem import ( "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" "testing" + "time" ) var leagues = map[string]League{ @@ -106,9 +111,8 @@ func TestCalculateTopRanks(t *testing.T) { cap float64 capped bool }{ - // TODO: Fix Capped - //{5, 605, 0, 0, 0, "little", 0, 1, 14, 337248, 0, 14, 15, 50, true}, - //{5, 605, 0, 0, 0, "little", 4, 5, 14, 333571, 1, 12, 15, 50, true}, + {5, 605, 0, 0, 0, "little", 0, 1, 14, 337248, 0, 14, 15, 50, true}, + {5, 605, 0, 0, 0, "little", 4, 5, 14, 333571, 1, 12, 15, 50, true}, {5, 605, 0, 0, 0, "great", 0, 1, 50, 1710113, 8, 15, 15, 50, false}, {5, 605, 0, 0, 0, "great", 10, 5, 50.5, 1709291, 7, 15, 15, 51, false}, } @@ -372,6 +376,28 @@ func TestFilterLevelCaps(t *testing.T) { } } +// TestFilterLevelCapsMerge ensures collapsed entries inherit Cap and Capped +// from later level caps (regression test for value-copy mutation bug). +func TestFilterLevelCapsMerge(t *testing.T) { + // Synthetic entries: same pokemon/level/rank across two caps. + // FilterLevelCaps should collapse the second into the first and update Cap+Capped. + entries := []PokemonEntry{ + {Pokemon: 100, Level: 30, Rank: 5, Cap: 50, Capped: false}, + {Pokemon: 100, Level: 30, Rank: 5, Cap: 51, Capped: true}, + } + ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} + out := ohbem.FilterLevelCaps(entries, []int{50, 51}) + if len(out) != 1 { + t.Fatalf("got %d entries, want 1", len(out)) + } + if out[0].Cap != 51 { + t.Errorf("got Cap=%v, want 51 (later cap should propagate)", out[0].Cap) + } + if !out[0].Capped { + t.Errorf("got Capped=false, want true (Capped from later entry must propagate)") + } +} + func BenchmarkFilterLevelCaps(b *testing.B) { ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} _ = ohbem.LoadPokemonData("./test/master-test.json") @@ -382,3 +408,104 @@ func BenchmarkFilterLevelCaps(b *testing.B) { _ = ohbem.FilterLevelCaps(entries["great"], []int{51}) } } + +// TestWatchPokemonDataRestart verifies start→stop→start works without +// returning ErrWatcherStarted or panicking on a re-stop. +func TestWatchPokemonDataRestart(t *testing.T) { + ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps, WatcherInterval: time.Hour} + if err := ohbem.LoadPokemonData("./test/master-test.json"); err != nil { + t.Fatalf("load: %v", err) + } + if err := ohbem.WatchPokemonData(); err != nil { + t.Fatalf("first start: %v", err) + } + if err := ohbem.StopWatchingPokemonData(); err != nil { + t.Fatalf("first stop: %v", err) + } + if err := ohbem.WatchPokemonData(); err != nil { + t.Fatalf("restart: %v", err) + } + if err := ohbem.StopWatchingPokemonData(); err != nil { + t.Fatalf("second stop: %v", err) + } + // Third stop must return error, not panic on closed channel. + if err := ohbem.StopWatchingPokemonData(); err != ErrNilChannel { + t.Errorf("expected ErrNilChannel on extra stop, got %v", err) + } +} + +// TestFetchPokemonDataNon200 verifies FetchPokemonData surfaces non-200 +// HTTP responses as ErrMasterFileFetch. +func TestFetchPokemonDataNon200(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "rate limited", http.StatusTooManyRequests) + })) + defer srv.Close() + ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps, MasterFileURL: srv.URL} + if err := ohbem.FetchPokemonData(); err != ErrMasterFileFetch { + t.Errorf("got %v, want ErrMasterFileFetch", err) + } +} + +// TestFetchPokemonDataMalformed verifies malformed JSON yields ErrMasterFileDecode. +func TestFetchPokemonDataMalformed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("{not json")) + })) + defer srv.Close() + ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps, MasterFileURL: srv.URL} + if err := ohbem.FetchPokemonData(); err != ErrMasterFileDecode { + t.Errorf("got %v, want ErrMasterFileDecode", err) + } +} + +// TestSavePokemonDataAtomic verifies SavePokemonData round-trips and that +// a path under a fresh tmp dir works. +func TestSavePokemonDataAtomic(t *testing.T) { + ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} + if err := ohbem.LoadPokemonData("./test/master-test.json"); err != nil { + t.Fatalf("load: %v", err) + } + dir := t.TempDir() + path := filepath.Join(dir, "out.json") + if err := ohbem.SavePokemonData(path); err != nil { + t.Fatalf("save: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("stat: %v", err) + } + other := Ohbem{Leagues: leagues, LevelCaps: levelCaps} + if err := other.LoadPokemonData(path); err != nil { + t.Fatalf("reload: %v", err) + } +} + +// FuzzQueryPvPRankBounds checks that out-of-range IV/level inputs return the +// documented sentinel error rather than panicking. +func FuzzQueryPvPRankBounds(f *testing.F) { + ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} + if err := ohbem.LoadPokemonData("./test/master-test.json"); err != nil { + f.Fatalf("load: %v", err) + } + f.Add(25, 0, 0, 1, 5, 5, 5, 1.0) + f.Add(25, 0, 0, 1, -1, 5, 5, 1.0) + f.Add(25, 0, 0, 1, 16, 5, 5, 1.0) + f.Add(25, 0, 0, 1, 5, 5, 5, 0.0) + f.Fuzz(func(t *testing.T, pid, form, costume, gender, a, d, s int, level float64) { + // Just ensure no panic; error vs success depends on inputs. + _, _ = ohbem.QueryPvPRank(pid, form, costume, gender, a, d, s, level) + }) +} + +// FuzzCalculateCpBounds is the equivalent fuzz for CalculateCp inputs. +func FuzzCalculateCpBounds(f *testing.F) { + ohbem := Ohbem{Leagues: leagues, LevelCaps: levelCaps} + if err := ohbem.LoadPokemonData("./test/master-test.json"); err != nil { + f.Fatalf("load: %v", err) + } + f.Add(25, 0, 0, 5, 5, 5, 1.0) + f.Add(25, 0, 0, -1, 5, 5, 1.0) + f.Fuzz(func(t *testing.T, pid, form, evolution, a, d, s int, level float64) { + _, _ = ohbem.CalculateCp(pid, form, evolution, a, d, s, level) + }) +} diff --git a/pvp_core.go b/pvp_core.go index 5b02f3a..427755e 100644 --- a/pvp_core.go +++ b/pvp_core.go @@ -2,9 +2,23 @@ package gohbem import ( "math" - "sort" + "slices" + "sync" ) +// rankArenaPool reuses the 4096-entry rank scratch buffer between calls. +// calculateRanksCompact returns the buffer to the caller; callers that don't +// need it long-term should release it via releaseRankArena. Cache stored +// values only need TopValue, so the buffer is short-lived in cached paths. +var rankArenaPool = sync.Pool{ + New: func() any { return new([4096]PvPRankingStats) }, +} + +func releaseRankArena(a *[4096]PvPRankingStats) { + *a = [4096]PvPRankingStats{} + rankArenaPool.Put(a) +} + // calculateCpMultiplier is used to calculate CP multiplier for provided level. It's using precalculated values from cpm.go file. func calculateCpMultiplier(level float64) float64 { intLevel := int(level * 2) @@ -71,47 +85,6 @@ func calculatePvPStat(out *PvPRankingStats, stats *PokemonStats, attack, defense return nil } -// calculateRanks is core method used to calculate PvP ranks for provided Pokemon data. -/* -func calculateRanks(stats *PokemonStats, cpCap int, lvCap float64, comparator RankingComparator) (*[16][16][16]Ranking, *[4096]*Ranking) { - combinations := new([16][16][16]Ranking) - sortedRanks := new([4096]*Ranking) - var c uint16 - - for a := 0; a <= 15; a++ { - for d := 0; d <= 15; d++ { - for s := 0; s <= 15; s++ { - currentStat, err := calculatePvPStat(stats, a, d, s, cpCap, lvCap, 1) - if err != nil { - continue - } - combinations[a][d][s] = currentStat - sortedRanks[c] = ¤tStat - c++ - } - } - } - - sort.SliceStable(sortedRanks, func(i, j int) bool { - return comparator(sortedRanks[i], sortedRanks[j]) < 0 - }) - - best := sortedRanks[0].Value - var i, j int16 - for i, j = 0, 0; i < int16(len(sortedRanks)); i++ { - entry := sortedRanks[i] - percentage := roundFloat(entry.Value/best, 5) - entry.Percentage = percentage - if entry.Value < sortedRanks[j].Value { - j = i - } - rank := j + 1 - entry.Rank = rank - } - return combinations, sortedRanks -} -*/ - // RankingComparatorDefault ranks everything by stat product descending then by attack descending. // This is the default behavior, since in general, a higher stat product is usually preferable; // and in case of tying stat products, higher attack means that you would be more likely to win CMP ties. @@ -165,49 +138,39 @@ func RankingComparatorPreferLowerCp(a, b *PvPRankingStats) int { return 0 } -type compactRankSorter struct { - ranks *[4096]PvPRankingStats - count int - comparator RankingComparator -} - -func (sorter compactRankSorter) Len() int { - return sorter.count -} - -func (sorter compactRankSorter) Less(i, j int) bool { - d := sorter.comparator(&sorter.ranks[i], &sorter.ranks[j]) - return d < 0 || d == 0 && sorter.ranks[i].Index < sorter.ranks[j].Index -} - -func (sorter compactRankSorter) Swap(i, j int) { - sorter.ranks[i], sorter.ranks[j] = sorter.ranks[j], sorter.ranks[i] -} - // calculateRanksCompact is optimized (for cache) core method used to calculate PvP ranks for provided Pokemon data. +// The returned [4096]PvPRankingStats array is drawn from a pool; callers that +// don't retain it should pass it back via releaseRankArena. func calculateRanksCompact(stats *PokemonStats, cpCap int, lvCap float64, comparator RankingComparator, ivFloor int) (*[4096]int16, *[4096]PvPRankingStats) { combinations := new([4096]int16) - sorter := compactRankSorter{ranks: new([4096]PvPRankingStats), comparator: comparator} + ranks := rankArenaPool.Get().(*[4096]PvPRankingStats) + count := 0 for a := ivFloor; a <= 15; a++ { for d := ivFloor; d <= 15; d++ { for s := ivFloor; s <= 15; s++ { - if calculatePvPStat(&sorter.ranks[sorter.count], stats, a, d, s, cpCap, lvCap, 1) == nil { - sorter.ranks[sorter.count].Index = (a*16+d)*16 + s - sorter.count++ + if calculatePvPStat(&ranks[count], stats, a, d, s, cpCap, lvCap, 1) == nil { + ranks[count].Index = (a*16+d)*16 + s + count++ } } } } - sort.Sort(sorter) + view := ranks[:count] + slices.SortFunc(view, func(a, b PvPRankingStats) int { + if d := comparator(&a, &b); d != 0 { + return d + } + return a.Index - b.Index + }) - for i, j := 0, 0; i < sorter.count; i++ { - entry := &sorter.ranks[i] - if comparator(&sorter.ranks[j], entry) < 0 { + for i, j := 0, 0; i < count; i++ { + entry := &ranks[i] + if comparator(&ranks[j], entry) < 0 { j = i } combinations[entry.Index] = int16(j + 1) } - return combinations, sorter.ranks + return combinations, ranks } diff --git a/pvp_core_test.go b/pvp_core_test.go index d2f1f4c..4fa9ef3 100644 --- a/pvp_core_test.go +++ b/pvp_core_test.go @@ -155,89 +155,6 @@ func BenchmarkCalculatePvPStat(b *testing.B) { } } -/* -func TestCalculateRanks(t *testing.T) { - var combinationTests = []struct { - stats PokemonStats - cpCap int - lvCap float64 - attack int - defense int - stamina int - value float64 - level float64 - cp int - percentage float64 - rank int16 - }{ - {PikachuStats, 100, 0, 0, 0, 0, 950.0466549389662, 1, 10, 0.69338, 4090}, - {PikachuStats, 10, 0, 0, 0, 0, 950.0466549389662, 1, 10, 0, 497}, - {PikachuStats, 600, 30, 0, 0, 0, 439598.41793819424, 29, 598, 0.92505, 1994}, - {PikachuStats, 600, 30, 15, 0, 0, 410559.4224700931, 25.5, 596, 0.86395, 4089}, - {PikachuStats, 600, 30, 15, 15, 0, 419733.0878105161, 23.5, 591, 0.88325, 3924}, - {PikachuStats, 600, 30, 15, 15, 15, 431674.6163042061, 22, 589, 0.90838, 2984}, - - {ElgyemStats, 100, 0, 0, 0, 0, 1700.04628357754, 1, 15, 0.68427, 4094}, - {ElgyemStats, 600, 30, 0, 0, 0, 405531.30261898035, 18.5, 590, 0.9177, 2959}, - {ElgyemStats, 600, 30, 15, 0, 0, 395597.85979182937, 17, 597, 0.89522, 3886}, - {ElgyemStats, 600, 30, 15, 15, 0, 394072.0167082542, 15.5, 584, 0.89177, 3951}, - {ElgyemStats, 600, 30, 15, 15, 15, 416491.5778971401, 15, 593, 0.9425, 1315}, - } - - var sortedTests = []struct { - stats PokemonStats - cpCap int - lvCap float64 - pos int - value float64 - level float64 - cp int - percentage float64 - rank int16 - }{ - {PikachuStats, 100, 10, 0, 32189.76897186037, 4.5, 100, 1, 1}, - {PikachuStats, 100, 10, 4095, 23253.65960055367, 4, 88, 0.72239, 4096}, - {PikachuStats, 600, 30, 1, 472634.34117978957, 26, 600, 0.99457, 2}, - {PikachuStats, 600, 30, 15, 468041.78255510365, 25.5, 598, 0.98491, 15}, - {PikachuStats, 600, 30, 100, 461712.0022201541, 26.5, 600, 0.97159, 101}, - {PikachuStats, 600, 30, 4095, 406700.0985435657, 25, 590, 0.85582, 4096}, - - {ElgyemStats, 100, 10, 0, 29115.735973629493, 3, 100, 1, 1}, - {ElgyemStats, 100, 10, 4095, 20145.311247780945, 2.5, 80, 0.6919, 4096}, - {ElgyemStats, 600, 30, 0, 441901.18212997954, 16.5, 600, 1, 1}, - {ElgyemStats, 600, 30, 4095, 382959.52940267365, 16.5, 584, 0.86662, 4096}, - } - - for ix, test := range combinationTests { - testName := fmt.Sprintf("combinations/%d", ix) - t.Run(testName, func(t *testing.T) { - combinations, _ := calculateRanks(test.stats, test.cpCap, test.lvCap) - ans := combinations[test.attack][test.defense][test.stamina] - if ans.Value != test.value || ans.Level != test.level || ans.Cp != test.cp || ans.Percentage != test.percentage || ans.Rank != test.rank { - t.Errorf("got %+v, want %+v", ans, test) - } - }) - } - - for ix, test := range sortedTests { - testName := fmt.Sprintf("sortedRanks/%d", ix) - t.Run(testName, func(t *testing.T) { - _, sortedRanks := calculateRanks(test.stats, test.cpCap, test.lvCap) - ans := sortedRanks[test.pos] - if ans.Value != test.value || ans.Level != test.level || ans.Cp != test.cp || ans.Percentage != test.percentage || ans.Rank != test.rank { - t.Errorf("got %+v, want %+v", ans, test) - } - }) - } -} - -func BenchmarkCalculateRanks(b *testing.B) { - for i := 0; i < b.N; i++ { - _, _ = calculateRanks(PikachuStats, 600, 30) - } -} -*/ - func TestCalculateRanksCompact(t *testing.T) { var combinationTests = []struct { cpCap int diff --git a/structs.go b/structs.go index 90c5bba..3f99c64 100644 --- a/structs.go +++ b/structs.go @@ -2,22 +2,30 @@ package gohbem import ( "sync" + "sync/atomic" "time" ) // Ohbem struct is holding main configuration, cache and channels. +// +// Concurrency: methods on *Ohbem are safe for concurrent use once the +// MasterFile has been loaded. Always pass *Ohbem rather than copying the +// value (which would copy the embedded mutex and atomic pointer). type Ohbem struct { PokemonData PokemonData LevelCaps []int Leagues map[string]League DisableCache bool + MasterFileURL string // when set, overrides default MasterFileURL MasterFileCachePath string // when provided: store there latest changed version of masterfile RankingComparator RankingComparator IncludeHundosUnderCap bool WatcherInterval time.Duration - compactRankCache sync.Map - watcherChan chan bool Logger Logger + + mu sync.RWMutex // guards PokemonData, RankingComparator, watcherChan + compactRankCache atomic.Pointer[sync.Map] // swappable so ClearCache cannot race readers + watcherChan chan bool } // Logger interface @@ -68,7 +76,6 @@ type Ranking struct { Stamina int `json:"stamina"` Cap float64 `json:"cap"` Capped bool `json:"capped,omitempty"` - Index int `json:"index,omitempty"` } // PokemonEntry is holding a row of result for QueryPvPRank and FilterLevelCaps functions. diff --git a/utils.go b/utils.go index f96890b..d767948 100644 --- a/utils.go +++ b/utils.go @@ -3,45 +3,58 @@ package gohbem import ( "encoding/json" "fmt" + "io" "math" "net/http" + "time" ) -// MasterFileURL is a remote address used to fetch MasterFile. +// MasterFileURL is default remote address used to fetch MasterFile. const MasterFileURL = "https://raw.githubusercontent.com/WatWowMap/Masterfile-Generator/master/master-latest-basics.json" +// masterFileMaxBytes caps the masterfile response size to guard against memory exhaustion. +// Real masterfile is ~1-2 MiB; 32 MiB is generous. +const masterFileMaxBytes = 32 << 20 + +// httpFetchTimeout is the timeout for masterfile HTTP requests. +const httpFetchTimeout = 30 * time.Second + +// roundFactor5 is 10^5 for the common roundFloat(x, 5) call site. +const roundFactor5 = 100000.0 + +// httpClient is reused across masterfile fetches to share TCP/TLS pools. +var httpClient = &http.Client{Timeout: httpFetchTimeout} + func roundFloat(val float64, precision uint) float64 { + if precision == 5 { + return math.Round(val*roundFactor5) / roundFactor5 + } ratio := math.Pow(10, float64(precision)) return math.Round(val*ratio) / ratio } -func containsInt(slice []int, value int) bool { - for _, v := range slice { - if v == value { - return true - } +func fetchMasterFile(url string) (PokemonData, error) { + if url == "" { + url = MasterFileURL } - return false -} - -func fetchMasterFile() (PokemonData, error) { - req, err := http.NewRequest("GET", MasterFileURL, nil) + req, err := http.NewRequest("GET", url, nil) if err != nil { return PokemonData{}, ErrMasterFileFetch } req.Header.Set("User-Agent", fmt.Sprintf("Gohbem/%s", VERSION)) - client := &http.Client{} - resp, err := client.Do(req) + resp, err := httpClient.Do(req) if err != nil { return PokemonData{}, ErrMasterFileFetch } - //goland:noinspection GoUnhandledErrorResult defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return PokemonData{}, ErrMasterFileFetch + } + var data PokemonData - err = json.NewDecoder(resp.Body).Decode(&data) - if err != nil { + if err := json.NewDecoder(io.LimitReader(resp.Body, masterFileMaxBytes)).Decode(&data); err != nil { return PokemonData{}, ErrMasterFileDecode } data.Initialized = true @@ -49,6 +62,11 @@ func fetchMasterFile() (PokemonData, error) { } func safetyCheck(o *Ohbem) error { + if o == nil { + return ErrMasterFileUnloaded + } + o.mu.RLock() + defer o.mu.RUnlock() if !o.PokemonData.Initialized { return ErrMasterFileUnloaded } From 65bd84a59763d6a92a702a031654c6979439fcfd Mon Sep 17 00:00:00 2001 From: lenisko <10072920+lenisko@users.noreply.github.com> Date: Tue, 5 May 2026 14:57:37 +0200 Subject: [PATCH 3/3] perf: recover hot-path regressions from race-fix commit - Revert slices.SortFunc back to sort.Sort over a sort.Interface adapter: the closure form forced &a/&b to escape per comparison, ballooning calculateRanksCompact from 3 to 110k allocs/call. With the adapter the 4096-entry sort runs in-place with no per-comparison allocs. - Add an atomic.Bool initialized flag mirrored under the write lock by Load/Fetch/Watch. safetyCheck now reads it lock-free instead of taking the RWMutex, and Leagues / LevelCaps are read lock-free (immutable post-init), cutting one RLock round-trip per call from FindBaseStats, IsMegaUnreleased, CalculateCp, and the QueryPvPRank entry guard. Bench (Apple M2 Pro, vs broken commit / vs original HEAD~1): - CalculateRanksCompact: 2.06 ms / 110k allocs -> 608 us / 3 allocs (matches original 595 us / 3 allocs) - CalculateTopRanks: 3.98 ms / 165k allocs -> 1.09 ms / 48 allocs (vs original 1.10 ms / 68 allocs; -29% allocs) - QueryPvPRank cold: 10.1 ms / 427k allocs -> 2.59 ms / 21 allocs (vs original 2.53 ms / 25 allocs; -16% allocs) - QueryPvPRankCached: 697 ns -> 690 ns (-12% vs original 784 ns) - FindBaseStats: 40 ns -> 29 ns (vs 17 ns; residual = 1 RLock) - IsMegaUnreleased: 25 ns -> 15 ns (vs 8 ns; residual = 1 RLock) go test -race ./... still passes. --- ohbem.go | 3 +++ pvp_core.go | 41 ++++++++++++++++++++++++++++------------- structs.go | 3 ++- utils.go | 11 +++++------ 4 files changed, 38 insertions(+), 20 deletions(-) diff --git a/ohbem.go b/ohbem.go index 29bd445..e927d3b 100644 --- a/ohbem.go +++ b/ohbem.go @@ -29,6 +29,7 @@ func (o *Ohbem) FetchPokemonData() error { o.RankingComparator = RankingComparatorDefault } o.mu.Unlock() + o.initialized.Store(true) o.ClearCache() return nil } @@ -50,6 +51,7 @@ func (o *Ohbem) LoadPokemonData(filePath string) error { o.RankingComparator = RankingComparatorDefault } o.mu.Unlock() + o.initialized.Store(true) o.ClearCache() return nil } @@ -134,6 +136,7 @@ func (o *Ohbem) WatchPokemonData() error { o.mu.Lock() o.PokemonData = pokemonData o.mu.Unlock() + o.initialized.Store(true) o.ClearCache() } } diff --git a/pvp_core.go b/pvp_core.go index 427755e..634b5c7 100644 --- a/pvp_core.go +++ b/pvp_core.go @@ -2,7 +2,7 @@ package gohbem import ( "math" - "slices" + "sort" "sync" ) @@ -138,34 +138,49 @@ func RankingComparatorPreferLowerCp(a, b *PvPRankingStats) int { return 0 } +// compactRankSorter is a sort.Interface adapter over a fixed-size rank arena. +// Using sort.Sort over this avoids per-comparison closure escapes that +// slices.SortFunc(..., func(a, b T) int) triggers when the comparator takes +// pointers to the value parameters. +type compactRankSorter struct { + ranks *[4096]PvPRankingStats + count int + comparator RankingComparator +} + +func (sorter compactRankSorter) Len() int { return sorter.count } + +func (sorter compactRankSorter) Less(i, j int) bool { + d := sorter.comparator(&sorter.ranks[i], &sorter.ranks[j]) + return d < 0 || d == 0 && sorter.ranks[i].Index < sorter.ranks[j].Index +} + +func (sorter compactRankSorter) Swap(i, j int) { + sorter.ranks[i], sorter.ranks[j] = sorter.ranks[j], sorter.ranks[i] +} + // calculateRanksCompact is optimized (for cache) core method used to calculate PvP ranks for provided Pokemon data. // The returned [4096]PvPRankingStats array is drawn from a pool; callers that // don't retain it should pass it back via releaseRankArena. func calculateRanksCompact(stats *PokemonStats, cpCap int, lvCap float64, comparator RankingComparator, ivFloor int) (*[4096]int16, *[4096]PvPRankingStats) { combinations := new([4096]int16) ranks := rankArenaPool.Get().(*[4096]PvPRankingStats) - count := 0 + sorter := compactRankSorter{ranks: ranks, comparator: comparator} for a := ivFloor; a <= 15; a++ { for d := ivFloor; d <= 15; d++ { for s := ivFloor; s <= 15; s++ { - if calculatePvPStat(&ranks[count], stats, a, d, s, cpCap, lvCap, 1) == nil { - ranks[count].Index = (a*16+d)*16 + s - count++ + if calculatePvPStat(&ranks[sorter.count], stats, a, d, s, cpCap, lvCap, 1) == nil { + ranks[sorter.count].Index = (a*16+d)*16 + s + sorter.count++ } } } } - view := ranks[:count] - slices.SortFunc(view, func(a, b PvPRankingStats) int { - if d := comparator(&a, &b); d != 0 { - return d - } - return a.Index - b.Index - }) + sort.Sort(sorter) - for i, j := 0, 0; i < count; i++ { + for i, j := 0, 0; i < sorter.count; i++ { entry := &ranks[i] if comparator(&ranks[j], entry) < 0 { j = i diff --git a/structs.go b/structs.go index 3f99c64..20b8d72 100644 --- a/structs.go +++ b/structs.go @@ -23,8 +23,9 @@ type Ohbem struct { WatcherInterval time.Duration Logger Logger - mu sync.RWMutex // guards PokemonData, RankingComparator, watcherChan + mu sync.RWMutex // guards PokemonData, RankingComparator, watcherChan compactRankCache atomic.Pointer[sync.Map] // swappable so ClearCache cannot race readers + initialized atomic.Bool // mirrors PokemonData.Initialized for lock-free safetyCheck watcherChan chan bool } diff --git a/utils.go b/utils.go index d767948..67f5ba1 100644 --- a/utils.go +++ b/utils.go @@ -61,13 +61,12 @@ func fetchMasterFile(url string) (PokemonData, error) { return data, nil } +// safetyCheck is hot-path: must not take the RWMutex. Initialized state is +// mirrored to o.initialized (atomic) by Load/Fetch/Watch under the write lock. +// Leagues and LevelCaps are configured at construction and not mutated after, +// so reading their length lock-free is safe. func safetyCheck(o *Ohbem) error { - if o == nil { - return ErrMasterFileUnloaded - } - o.mu.RLock() - defer o.mu.RUnlock() - if !o.PokemonData.Initialized { + if o == nil || !o.initialized.Load() { return ErrMasterFileUnloaded } if len(o.Leagues) == 0 {