Bugfix Issue 1666 Keyword XML Roundtrip Import - sebiboga/testlink-upgraded GitHub Wiki

Bugfix โ€” Issue #1666: XML round-trip keyword import wrongly rejected with 400 wrong_keywords_file

Issue: #1666 ยท Branch: fix/issue-1666 ยท Suite 1666 โ€” pre-fix 28/41, post-fix 41/41 PASS (php tmp/fixtures_1666.php && php tmp/suite_1666.php)

1. Symptom

Export a test project's keywords to XML, then re-import the file you just exported โ€” the normal way to merge a keyword update, and exactly what the dialog hint promises ("Existing keywords with the same name are updated; new ones are created"). The dialog answered 400 wrong_keywords_file โ€” "Wrong keywords file - the format could not be read" โ€” although the file was perfectly valid:

POST /api/keywordsxml/index.php/?action=import    (type=iSerializationToXML, own export)
=> 400 {"status":"error","message":"wrong_keywords_file","code":"wrong_keywords_file",
        "result":0,"imported":0,"skipped":0,"rows":0,"errors":[]}

rows:0 + errors:[] is the giveaway: the BFF claimed it read nothing at all.

Before: the dialog blames the file

2. Measured control matrix (pre-fix)

# File Answer Consequence
A the project's own export 400 wrong_keywords_file rows=0 merge workflow impossible
B all-new keyword 200 imported=1 passes only because the count grows
C update-only 400 wrong_keywords_file and the new notes are not written
D CSV round-trip 400 NO_KEYWORDS_IMPORTED, 2 ร— ALREADY_EXISTS same no-op, accurate message

No Event Viewer row in any case (log_level=16 audit lines only) โ€” a wrong boolean, not an exception. Console clean.

3. Root cause

  1. lib/functions/testproject.class.php::importKeywordsFromSimpleXML() returns one verdict for the whole file and keeps no per-row outcome (the CSV sibling has taken the optional $stats row report since #1605 โ€” the XML arm never got one).
  2. api/keywordsxml/index.php substituted the keyword COUNT DELTA for "rows read": $stats['rows'] = max(0,$after-$before).
  3. The guard then read "no growth" as "nothing happened": if ($result==tl::OK && $stats['imported']<=0 && $after===$before) $result=tl::ERROR;

A merge that only touches existing keywords is an update-in-place workload โ€” the count stays flat, so a valid file is refused. The converse (all-new) passes only by luck.

Not a regression of #1605: restoring the pre-#1605 api/keywordsxml/index.php from HEAD reproduces a byte-identical 400. #1605 only made the message more accurate, which exposed this pre-existing rejection.

4. The fix

importKeywordsFromSimpleXML() now takes the optional by-reference &$stats out-param (rows / imported / skipped / errors[{row,code,name}], capped at IMPORT_KEYWORD_ERRORS_MAX, the same cap as the CSV arm), driven by the real per-row writeToDB() results, and importKeywordsFromXMLFile() forwards it. The BFF drops the count proxy and gates only the XML arm on the reported rows:

if ($result == tl::OK && $stats['imported'] <= 0
    && ($type === 'iSerializationToXML' ? $stats['rows'] <= 0 : $after === $before)) {

A <keywords/> document with no <keyword> child still yields rows === 0 and is still refused with the legacy wrong_keywords_file code.

Rejected alternatives: reject only on $result != tl::OK (loses the safety net for an empty document); count audit_keyword_created events (events are written on the success path only, so an all-skipped file is indistinguishable from an empty one); fix tlKeyword::writeToDB() to UPDATE a duplicate (a different defect โ€” #1783 โ€” shared with keywordsedit/tcImport and needing a product decision).

Blast radius: the XML arm of both keyword-import entry points. The issue report claimed api/keywords/index.php was unaffected; a code review of this fix measured otherwise and the claim was corrected โ€” that route carried the same count-delta proxy (old api/keywords/index.php:421-424) and answered 422 EMPTY_FILE โ€” "The keywords file has no data rows" โ€” for a 4-row round trip, i.e. the same defect in a worse disguise. Fixed by the same two-line change. No screen posts to that route today (keywordsView.html opens the keywordsExport.html popup), so it was latent.

The CSV arms of both routes keep their own signals and their NO_KEYWORDS_IMPORTED / EMPTY_FILE codes byte for byte; the legacy return value of importKeywordsFromSimpleXML() is unchanged, so importKeywordsFromXML(), lib/testcases/tcImport.php and lib/testcases/tcCreateFromIssue.php behave exactly as in 1.9.20. No client change, no i18n change โ€” keywordsExport.html:360-364 already renders skipped > 0 as kwxml.partialImportMsg plus the per-row list, the same path the CSV partial import has used since #1605.

5. Result

Case Before After
re-import own export 400 wrong_keywords_file rows=0 200 rows=2 skipped=2, ALREADY_EXISTS per row
all-new XML 200 imported=1 unchanged
update-only XML 400, nothing written 200 rows=1 skipped=1 (no-op honestly reported โ€” #1783)
<keywords></keywords> 400 wrong_keywords_file unchanged
non-XML garbage / wrong root 400 wrong_keywords_file unchanged
<keyword> without name 400, no detail 400 + row 1 WRONG_FORMAT
CSV arm (all cases) โ€” unchanged
unknown project / id=0 / GET 404 / 400 / 405 unchanged

After: the dialog reports the real per-row outcome

Imported 0 of 5 rows; 5 row(s) were rejected - see the details below.
Row 1 (alpha): A keyword with this name already exists.

6. Found while testing โ€” filed, not fixed here

  • #1783 โ€” a keyword import never updates an existing keyword, XML and CSV: tlKeyword::writeToDB() (lib/functions/tlKeyword.class.php:190-213) returns E_NAMEALREADYEXISTS and skips its own UPDATE branch (:192) although checkKeyword() already resolved $this->dbID to the existing row (:230). The hint promises an update no code performs.
  • #1784 โ€” a malformed last <keyword> turns a successful XML import into 400 wrong_keywords_file although earlier rows were already written: the legacy $status is reassigned per row, so the last row decides the whole file.

7. Files

File Purpose
lib/functions/testproject.class.php importKeywordsFromSimpleXML() reports its per-row outcome; importKeywordsFromXMLFile() forwards $stats
api/keywordsxml/index.php count-delta proxy deleted; XML arm gated on the reported rows
api/keywords/index.php same proxy removed from the second import route (found by code review)
โš ๏ธ **GitHub.com Fallback** โš ๏ธ