From 758f7e8b35e8fa3d9fd35207ad48f9fabdcce671 Mon Sep 17 00:00:00 2001 From: russimicro Date: Sun, 26 Jul 2026 12:19:06 -0500 Subject: [PATCH] fix(engine): don't truncate a compound index expression to a field name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Table::field_index retries a lookup longer than 10 characters against name.substr(0, 10), because DBF CREATE truncates field names to 10 (a proc __output column 'databasepath' lands as DATABASEPA). An index expression whose first component is a 10-character field — CCODIGOCON+CDOCUMETRA, CPREFIJTRA+CDOCUMETRA — therefore matched that field and reported itself as a bare field name. AdsCreateIndex61 then took the bare-field branch and pinned the key length to the first component's width, so every later component fell out of the key: records sharing the first component collapsed onto one key and ordered by recno. GotoTop on the compound tag landed on the wrong record, and an exact AdsSeek matched the wrong row. The retry now only fires for a plain identifier (alphanumeric/underscore), which is the case it was written for. New test abi_cdx_estaelec_compound_test: two distinct compound tags over the same table (con+doc and pre+doc) must order differently, plus a computed DTOS() tag, a FOR-clause tag and an exact seek on a compound key. Found while running a Harbour/FiveWin ERP's real index set on OpenADS. Co-Authored-By: Claude Opus 5 (1M context) --- src/engine/table.cpp | 13 +- tests/CMakeLists.txt | 1 + tests/unit/abi_cdx_estaelec_compound_test.cpp | 220 ++++++++++++++++++ 3 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 tests/unit/abi_cdx_estaelec_compound_test.cpp diff --git a/src/engine/table.cpp b/src/engine/table.cpp index 3832b99c..ee186e76 100644 --- a/src/engine/table.cpp +++ b/src/engine/table.cpp @@ -13,6 +13,7 @@ #include "drivers/ntx/ntx_driver.h" #include +#include #include #include #include @@ -159,7 +160,17 @@ std::int32_t Table::field_index(const std::string& name) const noexcept { // (a proc __output column `databasepath` lands as DATABASEPA in // the temp free table), so a longer lookup name can only mean // the truncated field — retry with the storage-truncated form. - if (found < 0 && name.size() > 10) + // ONLY for a plain identifier: an index expression like + // `CCODIGOCON+CDOCUMETRA` starts with a 10-char field name, and + // truncating it would report the whole compound key as that bare + // field — pinning the key length to the first component's width, + // which collapses every later component out of the key. + const bool plain_ident = + !name.empty() && + std::all_of(name.begin(), name.end(), [](char c) { + return std::isalnum(static_cast(c)) || c == '_'; + }); + if (found < 0 && plain_ident && name.size() > 10) found = scan_for(name.substr(0, 10)); return found; }; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7d5788bb..b2b75c17 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -141,6 +141,7 @@ add_executable(openads_unit_tests unit/abi_cdx_empty_table_keylen_test.cpp unit/abi_cdx_recreate_tag_diff_expr_test.cpp unit/abi_cdx_multitag_create_test.cpp + unit/abi_cdx_estaelec_compound_test.cpp unit/abi_cdx_issue131_test.cpp unit/abi_ordnumber_test.cpp unit/abi_server_info_test.cpp diff --git a/tests/unit/abi_cdx_estaelec_compound_test.cpp b/tests/unit/abi_cdx_estaelec_compound_test.cpp new file mode 100644 index 00000000..019a2b49 --- /dev/null +++ b/tests/unit/abi_cdx_estaelec_compound_test.cpp @@ -0,0 +1,220 @@ +// Proof that the OpenADS CDX engine handles the ERP's exact index patterns — +// the ones the ADI driver could NOT (it indexes by field only, so compound / +// computed tags collide on the first field). +// +// This mirrors UTILIDAD.PRG IndexaTabla, table ESTAELEC: +// ORD1 cCodigoCon+cDocumeTra (compound concat) +// ORD2 cCodigoCli (single field) +// ORD3 cPreFijTra+cDocumeTra (compound concat — collides with ORD1 +// on field 0 in ADI; must stay distinct) +// ORD4 DTOS(dFecTraTra) (computed) +// ORD5 DTOS(dFecTraTra) FOR cCorEnvEle != 'S' (computed + conditional) +// +// If ORD1 and ORD3 produce DIFFERENT orderings and ORD5 only indexes the +// matching rows, the CDX engine supports what the migration needs and CDX on +// x64 (DBF + .cdx via OpenADS) is a viable replacement for the ADT/.adi route. + +#include "doctest.h" +#include "openads/ace.h" + +#include +#include +#include +#include + +namespace fs = std::filesystem; + +namespace { + +void set_c(ADSHANDLE h, const char* field, const char* val) { + UNSIGNED8 f[32]{}; std::strncpy(reinterpret_cast(f), field, 31); + UNSIGNED8 v[32]{}; std::strncpy(reinterpret_cast(v), val, 31); + REQUIRE(AdsSetString(h, f, v, + static_cast(std::strlen(val))) == AE_SUCCESS); +} + +// recno helper +UNSIGNED32 recno(ADSHANDLE h) { + UNSIGNED32 rn = 0; + REQUIRE(AdsGetRecordNum(h, 0, &rn) == AE_SUCCESS); + return rn; +} + +ADSHANDLE make_tag(ADSHANDLE hTable, const char* bag, const char* tag, + const char* expr, const char* cond) { + UNSIGNED8 b[260]{}; std::strncpy(reinterpret_cast(b), bag, 259); + UNSIGNED8 t[64]{}; std::strncpy(reinterpret_cast(t), tag, 63); + UNSIGNED8 e[128]{}; std::strncpy(reinterpret_cast(e), expr, 127); + UNSIGNED8 c[128]{}; + UNSIGNED8* cp = nullptr; + if (cond) { std::strncpy(reinterpret_cast(c), cond, 127); cp = c; } + ADSHANDLE h = 0; + REQUIRE(AdsCreateIndex61(hTable, b, t, e, cp, nullptr, 0, 0, &h) + == AE_SUCCESS); + return h; +} + +} // namespace + +TEST_CASE("CDX engine handles ESTAELEC compound/computed/conditional tags") { + fs::path dir = fs::temp_directory_path() / "openads_cdx_estaelec"; + std::error_code ec; fs::remove_all(dir, ec); fs::create_directories(dir); + + UNSIGNED8 srv[260]{}; + std::memcpy(srv, dir.string().c_str(), dir.string().size()); + ADSHANDLE hConn = 0; + REQUIRE(AdsConnect60(srv, ADS_LOCAL_SERVER, nullptr, nullptr, 0, &hConn) + == AE_SUCCESS); + + UNSIGNED8 tbl[] = "estaelec.dbf"; + UNSIGNED8 def[] = "CCODIGOCON,C,3,0;CDOCUMETRA,C,8,0;CCODIGOCLI,C,10,0;" + "CPREFIJTRA,C,4,0;DFECTRATRA,D,8,0;CCORENVELE,C,1,0"; + ADSHANDLE hT = 0; + REQUIRE(AdsCreateTable(hConn, tbl, nullptr, ADS_CDX, 0, 0, 0, 0, def, &hT) + == AE_SUCCESS); + + struct Row { const char* con; const char* doc; const char* cli; + const char* pre; const char* fec; const char* cor; }; + // Designed so ORD1 (con+doc) and ORD3 (pre+doc) order DIFFERENTLY. + const Row rows[4] = { + {"001", "00000010", "CLIENTE001", "FA01", "20260103", "S"}, // rec1 + {"002", "00000020", "CLIENTE002", "FA02", "20260101", "N"}, // rec2 + {"001", "00000030", "CLIENTE003", "FA03", "20260102", "S"}, // rec3 + {"003", "00000005", "CLIENTE004", "FA01", "20260104", "N"}, // rec4 + }; + for (const auto& r : rows) { + REQUIRE(AdsAppendRecord(hT) == AE_SUCCESS); + set_c(hT, "CCODIGOCON", r.con); + set_c(hT, "CDOCUMETRA", r.doc); + set_c(hT, "CCODIGOCLI", r.cli); + set_c(hT, "CPREFIJTRA", r.pre); + set_c(hT, "DFECTRATRA", r.fec); + set_c(hT, "CCORENVELE", r.cor); + REQUIRE(AdsWriteRecord(hT) == AE_SUCCESS); + } + + std::string bags = (dir / "estaelec.cdx").string(); + ADSHANDLE o1 = make_tag(hT, bags.c_str(), "ORD1", "CCODIGOCON+CDOCUMETRA", nullptr); + ADSHANDLE o2 = make_tag(hT, bags.c_str(), "ORD2", "CCODIGOCLI", nullptr); + ADSHANDLE o3 = make_tag(hT, bags.c_str(), "ORD3", "CPREFIJTRA+CDOCUMETRA", nullptr); + ADSHANDLE o4 = make_tag(hT, bags.c_str(), "ORD4", "DTOS(DFECTRATRA)", nullptr); + ADSHANDLE o5 = make_tag(hT, bags.c_str(), "ORD5", "DTOS(DFECTRATRA)", "CCORENVELE != 'S'"); + (void)o2; + + // All 5 tags coexist in the one .cdx bag. + UNSIGNED16 nidx = 0; + REQUIRE(AdsGetNumIndexes(hT, &nidx) == AE_SUCCESS); + CHECK(nidx == 5); + + // The ADI-failure proof: ORD1 and ORD3 are DISTINCT compound orders. + // ORD1 top key = "001"+"00000010" -> rec1. + REQUIRE(AdsSetIndexOrderByHandle(hT, o1) == AE_SUCCESS); + REQUIRE(AdsGotoTop(hT) == AE_SUCCESS); + CHECK(recno(hT) == 1u); + // ORD3 top key = "FA01"+"00000005" -> rec4 (different first record!). + REQUIRE(AdsSetIndexOrderByHandle(hT, o3) == AE_SUCCESS); + REQUIRE(AdsGotoTop(hT) == AE_SUCCESS); + CHECK(recno(hT) == 4u); + + // Computed order ORD4 = DTOS(date): earliest date is rec2 (20260101). + REQUIRE(AdsSetIndexOrderByHandle(hT, o4) == AE_SUCCESS); + REQUIRE(AdsGotoTop(hT) == AE_SUCCESS); + CHECK(recno(hT) == 2u); + + // Conditional ORD5 FOR cCorEnvEle != 'S' indexes only rec2 and rec4. + UNSIGNED32 kc = 0; + REQUIRE(AdsGetKeyCount(o5, 0, &kc) == AE_SUCCESS); + CHECK(kc == 2u); + + // Exact seek on the compound ORD1 lands on the right record. + REQUIRE(AdsSetIndexOrderByHandle(hT, o1) == AE_SUCCESS); + UNSIGNED8 key[] = "00100000030"; // "001"+"00000030" -> rec3 + UNSIGNED16 found = 0; + REQUIRE(AdsSeek(o1, key, 11, ADS_STRINGKEY, 0, &found) == AE_SUCCESS); + CHECK(found != 0); + CHECK(recno(hT) == 3u); + + REQUIRE(AdsCloseTable(hT) == AE_SUCCESS); + REQUIRE(AdsDisconnect(hConn) == AE_SUCCESS); + fs::remove_all(dir, ec); +} + +// Proves the CDX engine covers the cases the "CDX-over-ADT" reroute must handle +// beyond build-time: NUMERIC bare-field key, persistence across close/reopen, +// and MAINTENANCE (dbAppend re-evaluates the compound expression). If these pass, +// reusing CdxIndex for ADT tables is guaranteed sound for every key kind the ERP +// uses (not just the char/DTOS path of the test above). +TEST_CASE("CDX engine: numeric key + reopen persistence + dbAppend maintenance") { + fs::path dir = fs::temp_directory_path() / "openads_cdx_persist"; + std::error_code ec; fs::remove_all(dir, ec); fs::create_directories(dir); + + UNSIGNED8 srv[260]{}; + std::memcpy(srv, dir.string().c_str(), dir.string().size()); + ADSHANDLE hConn = 0; + REQUIRE(AdsConnect60(srv, ADS_LOCAL_SERVER, nullptr, nullptr, 0, &hConn) + == AE_SUCCESS); + + UNSIGNED8 tbl[] = "doc.dbf"; + UNSIGNED8 def[] = "CCODIGOCON,C,3,0;CDOCUMETRA,C,8,0;NSECUEN,N,4,0"; + ADSHANDLE hT = 0; + REQUIRE(AdsCreateTable(hConn, tbl, nullptr, ADS_CDX, 0, 0, 0, 0, def, &hT) + == AE_SUCCESS); + + struct R { const char* con; const char* doc; double seq; }; + const R rows[3] = { + {"001", "00000010", 30}, + {"002", "00000020", 10}, + {"001", "00000030", 20}, + }; + for (const auto& r : rows) { + REQUIRE(AdsAppendRecord(hT) == AE_SUCCESS); + set_c(hT, "CCODIGOCON", r.con); + set_c(hT, "CDOCUMETRA", r.doc); + REQUIRE(AdsSetDouble(hT, (UNSIGNED8*)"NSECUEN", r.seq) == AE_SUCCESS); + REQUIRE(AdsWriteRecord(hT) == AE_SUCCESS); + } + + std::string bags = (dir / "doc.cdx").string(); + ADSHANDLE oc = make_tag(hT, bags.c_str(), "ORDCOMP", "CCODIGOCON+CDOCUMETRA", nullptr); + ADSHANDLE on = make_tag(hT, bags.c_str(), "ORDNUM", "NSECUEN", nullptr); + (void)oc; + + // Numeric bare-field order: ascending by NSECUEN -> rec2(10), rec3(20), rec1(30). + REQUIRE(AdsSetIndexOrderByHandle(hT, on) == AE_SUCCESS); + REQUIRE(AdsGotoTop(hT) == AE_SUCCESS); + CHECK(recno(hT) == 2u); + + REQUIRE(AdsCloseTable(hT) == AE_SUCCESS); + + // Reopen: auto-binds doc.cdx. Orders must persist; select by NAME. + UNSIGNED8 t2[] = "doc.dbf"; + hT = 0; + REQUIRE(AdsOpenTable(hConn, t2, nullptr, ADS_CDX, 0, 0, 0, ADS_DEFAULT, &hT) + == AE_SUCCESS); + UNSIGNED16 nidx = 0; + REQUIRE(AdsGetNumIndexes(hT, &nidx) == AE_SUCCESS); + CHECK(nidx == 2); + + REQUIRE(AdsSetIndexOrder(hT, (UNSIGNED8*)"ORDCOMP") == AE_SUCCESS); + REQUIRE(AdsGotoTop(hT) == AE_SUCCESS); + CHECK(recno(hT) == 1u); // "001"+"00000010" + + // Maintenance: append a record; the compound order must place it correctly + // (sync_all_indexes_ re-evaluates CCODIGOCON+CDOCUMETRA on the new row). + REQUIRE(AdsAppendRecord(hT) == AE_SUCCESS); + set_c(hT, "CCODIGOCON", "000"); + set_c(hT, "CDOCUMETRA", "00000001"); + REQUIRE(AdsSetDouble(hT, (UNSIGNED8*)"NSECUEN", 99) == AE_SUCCESS); + REQUIRE(AdsWriteRecord(hT) == AE_SUCCESS); + + // The new row ("000"+"00000001") is the lowest compound key, so on ORDCOMP + // GotoTop must land on it (rec4) — proving the append was maintained in the + // compound order, not just stored by recno. + REQUIRE(AdsSetIndexOrder(hT, (UNSIGNED8*)"ORDCOMP") == AE_SUCCESS); + REQUIRE(AdsGotoTop(hT) == AE_SUCCESS); + CHECK(recno(hT) == 4u); + + REQUIRE(AdsCloseTable(hT) == AE_SUCCESS); + REQUIRE(AdsDisconnect(hConn) == AE_SUCCESS); + fs::remove_all(dir, ec); +}