From 02471a91cb967a1c94502f38b76586ecbd423cd2 Mon Sep 17 00:00:00 2001 From: Dario Gabriel Lipicar Date: Fri, 31 Jul 2026 18:22:31 -0300 Subject: [PATCH 1/2] fix(generator): the Qt consumer must not flatten a rejection into an empty value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A provider that REJECTS a call answers the canonical {"code":"dispatch_failed", "message":..., "origin":...} object as its RESULT, not as a transport error — the same object from every provider flavour (logos-qt-sdk dispatchFailedVariant, the generated cdylib dispatch, the Rust provider's args::dispatch_failed). The generated Qt consumer wrapper converted it like any other value, which ERASES it. `_result.toList()` on that map is `[]`, `.toString()` is "", `.toLongLong()` is 0 — so `echoUintList([1, -1, 3])`, answered `dispatch_failed` by the provider, reached the caller as `[]`: the whole list, not the bad element, and indistinguishable from "the provider returned nothing". Losing the error is a consumer bug whatever the provider did. Reported through the channel this surface already uses for a failed call: the `logos::CallError*` out-parameter every generated sync method carries (today "object_unavailable" when the target cannot be acquired). No signature changes, no return value changes — a caller that passes `err` now sees code="dispatch_failed" with the provider's diagnostic and origin; a caller that does not gets the same default it always got, plus the qWarning the wrapper already emits for a failed call. * the detector is emitted once per wrapper, in an anonymous namespace, and matches EXACTLY (three string fields, that code) for the same reason logos_rpc_status.h's isUnauthorizedSentinel matches exactly: an `any` or map return carrying user data must never false-match. * the result is now captured for `void` returns too — a void method can be rejected, and the rejection object is the only place that says so. * the async overload's callback takes the value alone and has no error parameter; giving it one would change the generated public surface (which logos-qt-sdk's veneer mirrors 1:1), so an async rejection is logged rather than delivered. Recorded in cpp-generator/docs/project.md. Scope: CONSUMER side only. The provider half of registry entry Q1 (a Qt-typed provider cannot validate the ELEMENT of a typed numeric array, because a C++ signature spells [uint] and [any] alike as QVariantList) is explicitly out of scope and unchanged. The lp wrapper is untouched — its generated output is byte-identical before and after. --- cpp-generator/docs/project.md | 13 ++++ cpp-generator/legacy/generator_lib.cpp | 92 +++++++++++++++++++++++--- tests/generator/test_make_source.cpp | 88 ++++++++++++++++++++++++ 3 files changed, 185 insertions(+), 8 deletions(-) diff --git a/cpp-generator/docs/project.md b/cpp-generator/docs/project.md index 7ad7019..6e7a6d0 100644 --- a/cpp-generator/docs/project.md +++ b/cpp-generator/docs/project.md @@ -320,3 +320,16 @@ Fixture files in `tests/experimental/fixtures/`: single-`_bytes`-field record is refused under both spellings. It used to read `f.type`, which refused `? _bytes: tstr` and let `_bytes: ?tstr` through — the same declaration, two answers. `?bstr` is unaffected either way: the tag lives in the value, not the slot. +- **A provider REJECTION reaches an async consumer callback only as a log line.** A + provider that refuses a call answers the canonical + `{"code":"dispatch_failed", "message":…, "origin":…}` object as its RESULT, not as a + transport error, and the Qt return table would convert it like any other value — + erasing it (`_result.toList()` on that map is `[]`). The Qt consumer emitter therefore + detects it and folds it into the `logos::CallError` out-parameter the sync wrapper + already carries, so `mod.echoUintList(v, &err)` can tell a rejection from an empty + return. The generated `…Async` overload has no such channel — its callback is + `std::function`, and adding an error parameter would change the generated + public surface (which logos-qt-sdk's `qt-generator --backend consumer` veneer mirrors + 1:1) — so an async rejection is reported with `qWarning` and the callback still + receives the default-converted value. Giving async an error channel is an API change, + not a code-generation fix. diff --git a/cpp-generator/legacy/generator_lib.cpp b/cpp-generator/legacy/generator_lib.cpp index f71a8c0..8de16f3 100644 --- a/cpp-generator/legacy/generator_lib.cpp +++ b/cpp-generator/legacy/generator_lib.cpp @@ -588,20 +588,82 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ return h; } +// The Qt consumer's rejection detector, emitted once per generated wrapper. +// +// A provider that REJECTS a call answers the canonical +// {"code":"dispatch_failed", "message":..., "origin":...} object as its RESULT, +// not as a transport error. Every provider flavour produces the same object +// (logos-qt-sdk `dispatchFailedVariant`, the generated cdylib dispatch, the Rust +// provider's `args::dispatch_failed`), and the Qt return table converts it like +// any other value — which ERASES it: `_result.toList()` on a map is `[]`, +// `.toString()` is "", `.toLongLong()` is 0. A caller then cannot tell "you sent +// me the wrong thing" from "the provider returned nothing". +// +// Detected here and folded into the logos::CallError out-channel the wrapper +// already uses to report a failed call, so a rejection reads exactly like every +// other failure on this surface — no new signature, no new type, and no change +// to any return value. +static void emitDispatchRejectionDetector(QTextStream& s) +{ + s << "namespace {\n\n"; + s << "// True when `v` is the canonical provider REJECTION object rather than a\n"; + s << "// value; fills `out` with its {code, message, origin} on a match.\n"; + s << "//\n"; + s << "// The match is exact — those three fields, all strings, and that code — for the\n"; + s << "// same reason logos_rpc_status.h's isUnauthorizedSentinel is exact: an `any` or\n"; + s << "// map return carrying user data must never false-match.\n"; + s << "bool logosDispatchRejection(const QVariant& v, logos::CallError& out)\n"; + s << "{\n"; + s << " QVariantMap m;\n"; + s << " switch (v.userType()) {\n"; + s << " case QMetaType::QVariantMap: m = v.toMap(); break;\n"; + s << " // Defensive: some json_convert paths historically produced QJsonObject.\n"; + s << " case QMetaType::QJsonObject: m = v.toJsonObject().toVariantMap(); break;\n"; + s << " default: return false;\n"; + s << " }\n"; + s << " if (m.size() != 3) return false;\n"; + s << " const QVariant code = m.value(QStringLiteral(\"code\"));\n"; + s << " const QVariant message = m.value(QStringLiteral(\"message\"));\n"; + s << " const QVariant origin = m.value(QStringLiteral(\"origin\"));\n"; + s << " if (code.userType() != QMetaType::QString\n"; + s << " || message.userType() != QMetaType::QString\n"; + s << " || origin.userType() != QMetaType::QString) return false;\n"; + s << " if (code.toString() != QStringLiteral(\"dispatch_failed\")) return false;\n"; + s << " out.code = code.toString().toStdString();\n"; + s << " out.message = message.toString().toStdString();\n"; + s << " out.origin = origin.toString().toStdString();\n"; + s << " return true;\n"; + s << "}\n\n"; + s << "} // namespace\n\n"; +} + QString makeSource(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& methods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) { if (apiStyle == ApiStyle::Lp) return makeSourceLp(moduleName, className, headerBaseName, methods, events, bindMode, records); const RecordSet rs = parseRecords(records); + // The rejection detector is only reachable from a method body, so a + // contract with no invokable method must not emit it (an unused function in + // an anonymous namespace is a -Wunused-function warning, and such a + // contract's wrapper stays byte-identical to what it generated before). + bool anyInvokable = false; + for (const QJsonValue& mv : methods) { + if (mv.toObject().value("isInvokable").toBool()) { anyInvokable = true; break; } + } QString c; QTextStream s(&c); s << "#include \"" << headerBaseName << "\"\n\n"; s << "#include \n"; - if (!rs.isEmpty()) { - // Record conversions build QVariantMaps. + if (!rs.isEmpty() || anyInvokable) { + // Record conversions build QVariantMaps; so does the rejection detector. s << "#include \n"; } + if (anyInvokable) { + // The rejection detector reads a QJsonObject-shaped result defensively. + s << "#include \n"; + } s << "\n"; + if (anyInvokable) emitDispatchRejectionDetector(s); emitRecordConversions(s, rs, apiStyle, className); // The expression every remote call uses to name its target module. // Static: the baked string literal "" (unchanged @@ -748,13 +810,15 @@ QString makeSource(const QString& moduleName, const QString& className, const QS // Body: perform call through the err-out overload. When the caller // passes a logos::CallError* it can distinguish a failed remote call - // (e.g. the bound module is missing) from a legitimately - // default-valued result; without it the historical default-on-failure - // behavior is kept, now with a warning so failures are at least - // visible in the module log. + // (e.g. the bound module is missing, or the provider REJECTED the + // arguments) from a legitimately default-valued result; without it the + // historical default-on-failure behavior is kept, now with a warning so + // failures are at least visible in the module log. + // + // The result is captured even for a `void` return: a void method can be + // rejected too, and the rejection object is the only place that says so. s << " logos::CallError _err;\n"; - if (ret != "void") s << " QVariant _result = "; - else s << " "; + s << " QVariant _result = "; // Wrap each argument in QVariant::fromValue so it becomes exactly ONE // element of the args list. A bare `QVariantList{v}` CONCATENATES a @@ -768,6 +832,11 @@ QString makeSource(const QString& moduleName, const QString& className, const QS if (i + 1 < params.size()) s << ", "; } s << "}, Timeout(), &_err);\n"; + // A provider REJECTION arrives as the result, not as a transport error. + // Fold it into the same error channel BEFORE the return table converts + // it, or the conversion erases it (a rejected `[uint]` call answered [] + // — the whole list, not the bad element). + s << " if (_err.ok()) logosDispatchRejection(_result, _err);\n"; s << " if (err) *err = _err;\n"; s << " else if (!_err.ok()) qWarning() << \"" << className << "::" << name << ": remote call failed:\" << QString::fromStdString(_err.message);\n"; @@ -836,6 +905,13 @@ QString makeSource(const QString& moduleName, const QString& className, const QS s << "}"; } s << ", [callback](QVariant v) {\n"; + // The async callback carries the value only — there is no CallError + // parameter to fill, and adding one would change the generated public + // surface. A rejection is at least made visible in the module log + // instead of vanishing into the return conversion below. + s << " { logos::CallError _rej; if (logosDispatchRejection(v, _rej))\n"; + s << " qWarning() << \"" << className << "::" << name + << "Async: remote call failed:\" << QString::fromStdString(_rej.message); }\n"; if (ret == "void") { s << " (void)v; callback();\n"; } else if (retIsRecord) { diff --git a/tests/generator/test_make_source.cpp b/tests/generator/test_make_source.cpp index e1ceafd..3a20516 100644 --- a/tests/generator/test_make_source.cpp +++ b/tests/generator/test_make_source.cpp @@ -292,3 +292,91 @@ TEST(MakeSourceTest, NonInvokableSkipped) QString src = makeSource("mod", "Mod", "mod.h", methods); EXPECT_FALSE(src.contains("hidden")); } + +// ─── The provider REJECTION envelope on the return path ───────────────────── +// +// A provider that refuses a call answers the canonical +// {"code":"dispatch_failed", "message":…, "origin":…} object as its RESULT. The +// Qt return table converts it like any other value, which ERASES it — a rejected +// `[uint]` call answered `[]`, indistinguishable from "the provider returned +// nothing". These pin the consumer folding it into the CallError out-channel the +// wrapper already uses for a failed call. + +TEST(MakeSourceTest, QtEmitsRejectionDetector) +{ + QJsonArray methods; + methods.append(makeMethod("fn", "QVariantList", 1)); + QString src = makeSource("mod", "Mod", "mod.h", methods); + EXPECT_TRUE(src.contains("bool logosDispatchRejection(const QVariant& v, logos::CallError& out)")); + // Exact match only: an `any` / map return carrying user data must not + // false-match (same discipline as logos_rpc_status.h's sentinel). + EXPECT_TRUE(src.contains("if (m.size() != 3) return false;")); + EXPECT_TRUE(src.contains("if (code.toString() != QStringLiteral(\"dispatch_failed\")) return false;")); +} + +TEST(MakeSourceTest, QtSyncFoldsRejectionIntoCallError) +{ + QJsonArray methods; + methods.append(makeMethod("echoUintList", "QVariantList", 1)); + QString src = makeSource("mod", "Mod", "mod.h", methods); + // Folded BEFORE the return table converts the value, and before *err is + // written, so a caller passing err sees the rejection. + const int fold = src.indexOf("if (_err.ok()) logosDispatchRejection(_result, _err);"); + const int assign = src.indexOf("if (err) *err = _err;"); + const int convert = src.indexOf("return _result.toList();"); + EXPECT_NE(fold, -1); + EXPECT_NE(assign, -1); + EXPECT_NE(convert, -1); + EXPECT_LT(fold, assign); + EXPECT_LT(assign, convert); +} + +TEST(MakeSourceTest, QtVoidReturnStillCapturesResult) +{ + // A void method can be rejected too, and the rejection object is the only + // place that says so — so the result has to be captured even when it is + // never returned. + QJsonArray methods; + methods.append(makeMethod("doVoid", "void", 0)); + QString src = makeSource("mod", "Mod", "mod.h", methods); + EXPECT_TRUE(src.contains("QVariant _result = m_client->invokeRemoteMethod(\"mod\", \"doVoid\"")); + EXPECT_TRUE(src.contains("if (_err.ok()) logosDispatchRejection(_result, _err);")); +} + +TEST(MakeSourceTest, QtAsyncLogsRejection) +{ + // The async callback takes the value alone — there is no CallError to fill + // without changing the generated public surface — so the rejection has to at + // least reach the module log instead of vanishing into the conversion. + QJsonArray methods; + methods.append(makeMethod("fn", "QVariantList", 1)); + QString src = makeSource("mod", "Mod", "mod.h", methods); + EXPECT_TRUE(src.contains("{ logos::CallError _rej; if (logosDispatchRejection(v, _rej))")); + EXPECT_TRUE(src.contains("Mod::fnAsync: remote call failed:")); +} + +TEST(MakeSourceTest, QtNoInvokableMethodsEmitsNoDetector) +{ + // Unreachable from any body: emitting it would be an unused function in an + // anonymous namespace (-Wunused-function), and such a contract's wrapper + // stays byte-identical to what it generated before. + QJsonArray methods; + QJsonObject m; + m["name"] = "hidden"; + m["returnType"] = "void"; + m["isInvokable"] = false; + m["parameters"] = QJsonArray(); + methods.append(m); + QString src = makeSource("mod", "Mod", "mod.h", methods); + EXPECT_FALSE(src.contains("logosDispatchRejection")); +} + +TEST(MakeSourceTest, LpSurfaceIsUntouched) +{ + // The fix is Qt-consumer-only; the lp wrapper must generate exactly as + // before (byte-identical output is the negative control for the change). + QJsonArray methods; + methods.append(makeMethod("fn", "QVariantList", 1)); + QString src = makeSource("mod", "Mod", "mod.h", methods, ApiStyle::Lp); + EXPECT_FALSE(src.contains("logosDispatchRejection")); +} From 4223788a434c3efc827dd31bce92a4090250733f Mon Sep 17 00:00:00 2001 From: Dario Gabriel Lipicar Date: Fri, 31 Jul 2026 18:29:07 -0300 Subject: [PATCH 2/2] =?UTF-8?q?fix(generator):=20guard=20the=20emitted=20d?= =?UTF-8?q?etector=20=E2=80=94=20the=20umbrella=20is=20one=20translation?= =?UTF-8?q?=20unit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `logos_sdk.cpp` textually `#include`s every generated `_api.cpp`, so a module with more than one dependency compiles several copies of the detector into ONE translation unit: test_fullapi_rust_api.cpp:15:6: error: redefinition of 'logosDispatchRejection' Internal linkage covers the separate-TU case; only the preprocessor covers this one. Found by building test_fullapi_qtproxy (3 wrappers: two concrete deps plus the bound `full_api` interface) against this generator — a single-wrapper contract cannot reach it. --- cpp-generator/legacy/generator_lib.cpp | 7 +++++++ tests/generator/test_make_source.cpp | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/cpp-generator/legacy/generator_lib.cpp b/cpp-generator/legacy/generator_lib.cpp index 8de16f3..7a98208 100644 --- a/cpp-generator/legacy/generator_lib.cpp +++ b/cpp-generator/legacy/generator_lib.cpp @@ -603,8 +603,14 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ // already uses to report a failed call, so a rejection reads exactly like every // other failure on this surface — no new signature, no new type, and no change // to any return value. +// Guarded because the umbrella (`logos_sdk.cpp`) textually #includes EVERY +// generated `_api.cpp`, so a module with more than one dependency puts +// several of these in ONE translation unit. Internal linkage handles the +// separate-TU case; only the preprocessor handles this one. static void emitDispatchRejectionDetector(QTextStream& s) { + s << "#ifndef LOGOS_GENERATED_DISPATCH_REJECTION\n"; + s << "#define LOGOS_GENERATED_DISPATCH_REJECTION\n\n"; s << "namespace {\n\n"; s << "// True when `v` is the canonical provider REJECTION object rather than a\n"; s << "// value; fills `out` with its {code, message, origin} on a match.\n"; @@ -635,6 +641,7 @@ static void emitDispatchRejectionDetector(QTextStream& s) s << " return true;\n"; s << "}\n\n"; s << "} // namespace\n\n"; + s << "#endif // LOGOS_GENERATED_DISPATCH_REJECTION\n\n"; } QString makeSource(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& methods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) diff --git a/tests/generator/test_make_source.cpp b/tests/generator/test_make_source.cpp index 3a20516..10f5e33 100644 --- a/tests/generator/test_make_source.cpp +++ b/tests/generator/test_make_source.cpp @@ -380,3 +380,22 @@ TEST(MakeSourceTest, LpSurfaceIsUntouched) QString src = makeSource("mod", "Mod", "mod.h", methods, ApiStyle::Lp); EXPECT_FALSE(src.contains("logosDispatchRejection")); } + +TEST(MakeSourceTest, QtRejectionDetectorIsPreprocessorGuarded) +{ + // The umbrella (logos_sdk.cpp) textually #includes EVERY generated + // `_api.cpp`, so a module with more than one dependency puts several + // copies in ONE translation unit — "redefinition of logosDispatchRejection". + // Internal linkage does not help there; only the guard does. + QJsonArray methods; + methods.append(makeMethod("fn", "QVariantList", 1)); + QString src = makeSource("mod", "Mod", "mod.h", methods); + EXPECT_TRUE(src.contains("#ifndef LOGOS_GENERATED_DISPATCH_REJECTION")); + EXPECT_TRUE(src.contains("#define LOGOS_GENERATED_DISPATCH_REJECTION")); + EXPECT_TRUE(src.contains("#endif // LOGOS_GENERATED_DISPATCH_REJECTION")); + // Concatenating two generated wrappers, as the umbrella does, must compile: + // the second copy is preprocessed away. + QString other = makeSource("dep", "Dep", "dep.h", methods); + EXPECT_EQ(other.count("bool logosDispatchRejection"), 1); + EXPECT_EQ((src + other).count("#ifndef LOGOS_GENERATED_DISPATCH_REJECTION"), 2); +}