fix(learner): harden import/merge for clean multi-player data merge

Importing learned data merged from several players must not corrupt the store. Fixes: - ValidateImport prefix check was a no-op: parsed as (always false), so non-Questie strings were never rejected. Now a proper inequality check. - MergeImport now ensures the learner stores exist (fresh profile can import), merges each entry synchronously via _ApplyIncomingNetworkMerge (validates structure, only adopts missing fields, never overwrites good local data), wraps each entry in pcall so one malformed entry is skipped/counted rather than aborting the whole import, and returns accurate merged/skipped/rejected counts. - Synchronous merge also fixes InjectLearnedData previously running before the async-queued merges landed. selene 0 errors; busted 145 successes / same 7 pre-existing failures.
2026-06-10 05:12:04 -05:00
parent 1b06808655
commit 2c2e4a24a7
2 changed files with 46 additions and 14 deletions
@@ -34,6 +34,7 @@

 ### Bug Fixes

+- **[Learner - Hardened Import/Merge For Multi-Player Data]** Importing learned data merged from several different players is now safe and clean. Fixed a prefix-validation bug in `ValidateImport` (`not str:sub(...) == x` parsed as `(not str:sub(...)) == x`, always false, so non-Questie strings were never rejected). `MergeImport` now: ensures the learner stores exist (so a fresh profile can import), merges each entry **synchronously and defensively** via `_ApplyIncomingNetworkMerge` (which validates key/coordinate structure and only adopts fields the local store is missing — never overwriting good local data), isolates every entry in a `pcall` so one corrupt entry is skipped/counted instead of aborting the import or corrupting the store, and reports accurate `merged / skipped / rejected` counts. Merging synchronously also fixes `InjectLearnedData` previously running before the queued merges landed.
 - **[Quest - Turn-In '?' Missing For Quests In The Log]** A quest in the player's log is active and not yet turned in, so its turn-in `?` finisher should always be drawable — but `AddFinisher` also required `not char.complete[questId]`, so a quest completed in a previous Ascension prestige (still flagged in `char.complete`) and then re-accepted had its finisher suppressed while standing at the turn-in NPC. `AddFinisher` now trusts the live quest log: if the quest is in `currentQuestlog` and not failed, the finisher draws regardless of the (possibly stale) completed flag. No quest-completion data is modified.
 - **[Learner - Turn-In/Quest NPC Not Learned When Targeted]** In learner mode the turn-in NPC's location wasn't always recorded, so its `?` finisher couldn't draw. `OnTargetChanged` only cached the GUID and never called `LearnNPC`; it now learns the spawn of quest-giver/turn-in NPCs (gated like `OnMouseoverUnit`), so simply targeting a turn-in NPC records its position. The `QUEST_COMPLETE`/`QUEST_TURNED_IN` handlers also fall back to the `target` unit when the `npc` gossip unit is already cleared, so the finisher NPC is reliably learned on turn-in.
 - **[Map - Completed Quests Still Shown As Available]** (#7) The server's completed-quest list is delivered asynchronously (the `QUEST_QUERY_COMPLETE` event), often after available quests were first drawn — so quests that were actually already complete kept showing as available `!` until something forced a redraw (e.g. `/reload`). Questie now recalculates available quests once `char.complete` is populated by that event, removing the completed ones. This also clears the "already completed" subset of the false-available pins reported in #8.
@@ -199,9 +199,10 @@ function QuestieLearnerExport:ValidateImport(importStr)
    -- Strip whitespace
    importStr = importStr:gsub("%s+", "")

-    -- Check prefix
-    if not importStr:sub(1, #FORMAT_PREFIX + 2) == FORMAT_PREFIX .. ":" then
-        return nil, "Not a Questie-X export string (missing QxLD prefix)."
+    -- Check prefix (e.g. "QxLD:"). NOTE: the previous check `not str:sub(...) == x` parsed as
+    -- `(not str:sub(...)) == x` which is always false, so invalid strings were never rejected.
+    if importStr:sub(1, #FORMAT_PREFIX + 1) ~= (FORMAT_PREFIX .. ":") then
+        return nil, "Not a Questie-X export string (missing " .. FORMAT_PREFIX .. " prefix)."
    end

    local sepPos = importStr:find(FORMAT_SEP, 1, true)
@@ -267,19 +268,48 @@ function QuestieLearnerExport:MergeImport()

    local payload = self.lastImportData
    local bucket  = payload.data
-    local merged  = 0
-    local skipped = 0

+    -- Ensure the learner stores exist before merging so a fresh/empty profile can still
+    -- accept an import (and so per-type merges never index a nil table).
+    local g = Questie.dbLearner and Questie.dbLearner.global
+    if not g then
+        return false, "Learner data is not initialized."
+    end
+    g.npcs    = g.npcs    or {}
+    g.quests  = g.quests  or {}
+    g.items   = g.items   or {}
+    g.objects = g.objects or {}
+    g.settings = g.settings or {}
+
+    local merged   = 0
+    local skipped  = 0
+    local rejected = 0
+
+    -- Apply each entry SYNCHRONOUSLY and DEFENSIVELY so importing data merged from several
+    -- different players is safe:
+    --  * each entry is validated for key/coordinate structure inside
+    --    _ApplyIncomingNetworkMerge (via _ValidateLearnedSpawnData) and only adopts fields
+    --    the local store is missing — it never overwrites good local data;
+    --  * a single malformed entry (corrupt coords, wrong types) is caught by pcall and
+    --    skipped/counted instead of aborting the whole import or corrupting the store;
+    --  * we merge synchronously (not via the async comms queue) so InjectLearnedData below
+    --    sees the merged data and the returned counts are accurate.
    local function MergeType(typ, src)
+        if type(src) ~= "table" then return end
        for id, d in pairs(src) do
-            local prevData = QuestieLearner.data
-            QuestieLearner:HandleNetworkData(typ, id, d)
-            if QuestieLearner.data ~= prevData then
-                merged = merged + 1
+            local nid = tonumber(id) or id
+            if type(nid) == "number" and nid > 0 and type(d) == "table" then
+                local ok, applied = pcall(QuestieLearner._ApplyIncomingNetworkMerge, QuestieLearner, typ, nid, d)
+                if ok and applied then
+                    merged = merged + 1
+                elseif ok then
+                    skipped = skipped + 1
+                else
+                    rejected = rejected + 1
+                end
            else
-                skipped = skipped + 1
+                rejected = rejected + 1
            end
-            merged = merged + 1
        end
    end

@@ -292,12 +322,13 @@ function QuestieLearnerExport:MergeImport()
    self.lastImportStats = nil

    -- Push merged data into QuestieDB overrides immediately (no reload required for override data)
-    local QuestieLearner = QuestieLoader:ImportModule("QuestieLearner")
    if QuestieLearner and QuestieLearner.InjectLearnedData then
-        QuestieLearner:InjectLearnedData()
+        pcall(QuestieLearner.InjectLearnedData, QuestieLearner)
    end

-    local msg = "Import complete: merged " .. merged .. " entries, skipped " .. skipped .. " (already known)."
+    local msg = string.format("Import complete: merged %d, skipped %d already-known%s.",
+        merged, skipped,
+        rejected > 0 and (", rejected " .. rejected .. " malformed") or "")
    Questie:Debug(Questie.DEBUG_DEVELOP, "[LearnerExport]", msg)
    return true, msg
 end