diff --git a/CHANGELOG.md b/CHANGELOG.md index f2a31a4..2519f06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,9 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ## [Unreleased] -### Added +### Fixed -- (none yet — next changes land here) +- **`ReactiveListModel` / `ReactiveObject`: defer the initial fetch to `componentComplete()`.** Both classes now implement `QQmlParserStatus` and only fire the first `refresh()` from `componentComplete()` instead of inline from `setBaseUrl()` / `setSource()`. The pre-fix behaviour fired the GET as soon as the second of {`baseUrl`, `source`} was set — and because QML evaluates literal property assignments before bindings to other objects' properties, a model declared with literal `source` + bindings to `BackendConnection.url` / `BackendConnection.token` could fire its GET *before* the `token` binding had landed. The unauthenticated request hit Symfony's `SessionAuthenticator`, returned 401, and the model parked at `ready === false` with an empty list. Mercure subscribed anonymously (the model explicitly sets the SSE client's bearer to `""`), so subsequent server-side mutations propagated fine — masking the initial-fetch failure as "list is empty until something changes". Most visible when opening a second window via `make:bridge:window` after the first window's bindings had populated `BackendConnection`. After componentComplete, individual setter changes still trigger refresh inline as before, so token rotation / URL changes after first load behave unchanged. Regression test under [`framework/qml/tests/tst_reactive_list_model.qml`](framework/qml/tests/tst_reactive_list_model.qml) using the v0.2.0 `qmltestrunner` harness; added a `TestHttpServer` helper in the test scope that mimics `SessionAuthenticator`'s 401-on-no-bearer behaviour so the regression is observable as `ready === false` + empty `lastAuthHeader`. ## [0.2.0] — 2026-05-03 diff --git a/framework/qml/src/ReactiveListModel.cpp b/framework/qml/src/ReactiveListModel.cpp index 763fa61..541912c 100644 --- a/framework/qml/src/ReactiveListModel.cpp +++ b/framework/qml/src/ReactiveListModel.cpp @@ -28,13 +28,21 @@ ReactiveListModel::~ReactiveListModel() qDeleteAll(m_echoTimers); } +void ReactiveListModel::componentComplete() +{ + m_complete = true; + if (!m_baseUrl.isEmpty() && !m_source.isEmpty()) { + refresh(); + } +} + void ReactiveListModel::setBaseUrl(const QString& v) { if (m_baseUrl == v) return; m_baseUrl = v; rewireMercure(); emit baseUrlChanged(); - if (!m_source.isEmpty()) refresh(); + if (m_complete && !m_source.isEmpty()) refresh(); } void ReactiveListModel::setToken(const QString& v) @@ -50,7 +58,7 @@ void ReactiveListModel::setSource(const QString& v) if (m_source == v) return; m_source = v; emit sourceChanged(); - if (!m_baseUrl.isEmpty()) refresh(); + if (m_complete && !m_baseUrl.isEmpty()) refresh(); } void ReactiveListModel::setTopic(const QString& v) diff --git a/framework/qml/src/ReactiveListModel.h b/framework/qml/src/ReactiveListModel.h index 7e01e57..7079552 100644 --- a/framework/qml/src/ReactiveListModel.h +++ b/framework/qml/src/ReactiveListModel.h @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -27,9 +28,10 @@ class MercureClient; /// version-gap detection. Cursor pagination is wired but the default /// "fetch everything" behaviour is fine for small collections; bigger /// resources should set `pageSize` and call `fetchMore()` from the view. -class ReactiveListModel : public QAbstractListModel +class ReactiveListModel : public QAbstractListModel, public QQmlParserStatus { Q_OBJECT + Q_INTERFACES(QQmlParserStatus) QML_ELEMENT Q_PROPERTY(QString baseUrl READ baseUrl WRITE setBaseUrl NOTIFY baseUrlChanged) @@ -43,6 +45,16 @@ public: explicit ReactiveListModel(QObject* parent = nullptr); ~ReactiveListModel() override; + // QQmlParserStatus — lets us defer the initial fetch until ALL + // bindings have landed. Without this, a setter that sees enough + // state to fetch (baseUrl + source) can fire `refresh()` before + // the binding for `token` has run, sending an unauthenticated GET + // and parking an empty model. componentComplete() is the single + // safe trigger for the first fetch; later setter changes still + // fire refresh() inline as before. + void classBegin() override {} + void componentComplete() override; + // QAbstractListModel int rowCount(const QModelIndex& parent = QModelIndex()) const override; QVariant data(const QModelIndex& index, int role) const override; @@ -121,6 +133,7 @@ private: QString m_source; QString m_topic; bool m_ready = false; + bool m_complete = false; // QQmlParserStatus marker QString m_error; QNetworkAccessManager* m_nam = nullptr; diff --git a/framework/qml/src/ReactiveObject.cpp b/framework/qml/src/ReactiveObject.cpp index d16adac..06bd1e0 100644 --- a/framework/qml/src/ReactiveObject.cpp +++ b/framework/qml/src/ReactiveObject.cpp @@ -29,13 +29,21 @@ ReactiveObject::~ReactiveObject() qDeleteAll(m_echoTimers); } +void ReactiveObject::componentComplete() +{ + m_complete = true; + if (!m_baseUrl.isEmpty() && !m_source.isEmpty()) { + refresh(); + } +} + void ReactiveObject::setBaseUrl(const QString& v) { if (m_baseUrl == v) return; m_baseUrl = v; rewireMercure(); emit baseUrlChanged(); - if (!m_source.isEmpty()) refresh(); + if (m_complete && !m_source.isEmpty()) refresh(); } void ReactiveObject::setToken(const QString& v) @@ -50,7 +58,7 @@ void ReactiveObject::setSource(const QString& v) if (m_source == v) return; m_source = v; emit sourceChanged(); - if (!m_baseUrl.isEmpty()) refresh(); + if (m_complete && !m_baseUrl.isEmpty()) refresh(); } void ReactiveObject::setTopic(const QString& v) diff --git a/framework/qml/src/ReactiveObject.h b/framework/qml/src/ReactiveObject.h index 1d2aad5..e0f3fcb 100644 --- a/framework/qml/src/ReactiveObject.h +++ b/framework/qml/src/ReactiveObject.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -26,9 +27,10 @@ class MercureClient; /// ReactiveListModel.invoke(): apply locally + Idempotency-Key + roll /// back on `4xx`/`5xx`/timeout, clear `pending` on the matching /// Mercure echo (PLAN.md §5). -class ReactiveObject : public QObject +class ReactiveObject : public QObject, public QQmlParserStatus { Q_OBJECT + Q_INTERFACES(QQmlParserStatus) QML_ELEMENT Q_PROPERTY(QString baseUrl READ baseUrl WRITE setBaseUrl NOTIFY baseUrlChanged) @@ -45,6 +47,13 @@ public: explicit ReactiveObject(QObject* parent = nullptr); ~ReactiveObject() override; + // QQmlParserStatus — defer the initial fetch to componentComplete() + // so the GET goes out with token + baseUrl + source all populated, + // regardless of which order QML evaluated the bindings. See the + // matching note on ReactiveListModel. + void classBegin() override {} + void componentComplete() override; + QString baseUrl() const { return m_baseUrl; } void setBaseUrl(const QString& v); @@ -110,9 +119,10 @@ private: QString m_token; QString m_source; QString m_topic; - bool m_ready = false; - bool m_pending = false; - bool m_exists = false; + bool m_ready = false; + bool m_pending = false; + bool m_exists = false; + bool m_complete = false; // QQmlParserStatus marker QString m_error; QQmlPropertyMap* m_data = nullptr; diff --git a/framework/qml/tests/CMakeLists.txt b/framework/qml/tests/CMakeLists.txt index 63e1996..21c3ac0 100644 --- a/framework/qml/tests/CMakeLists.txt +++ b/framework/qml/tests/CMakeLists.txt @@ -8,13 +8,36 @@ # # Or from the skeleton / example Makefiles via `make qmltest`. -find_package(Qt6 6.5 REQUIRED COMPONENTS QuickTest) +find_package(Qt6 6.5 REQUIRED COMPONENTS QuickTest Network) + +# A tiny PhpQml.Bridge.Tests QML module that exposes the in-process +# stub HTTP server used by tst_reactive_list_model.qml. Static so it +# links into the test exe alongside the production bridge module. +qt_add_qml_module(php_qml_bridge_tests + URI PhpQml.Bridge.Tests + VERSION 1.0 + STATIC + OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/PhpQml/Bridge/Tests + SOURCES + TestHttpServer.h + TestHttpServer.cpp +) + +target_link_libraries(php_qml_bridge_tests PUBLIC + Qt6::Core + Qt6::Network + Qt6::Qml +) qt_add_executable(qml_unit_tests main.cpp) target_link_libraries(qml_unit_tests PRIVATE Qt6::QuickTest Qt6::Qml Qt6::Quick + php_qml_bridge # production module — type implementations + php_qml_bridgeplugin # …and its auto-generated QQmlEngineExtensionPlugin + php_qml_bridge_tests # in-process HTTP stub + php_qml_bridge_testsplugin # …and its plugin ) # QUICK_TEST_MAIN reads QUICK_TEST_SOURCE_DIR from the macro definition diff --git a/framework/qml/tests/TestHttpServer.cpp b/framework/qml/tests/TestHttpServer.cpp new file mode 100644 index 0000000..fb32358 --- /dev/null +++ b/framework/qml/tests/TestHttpServer.cpp @@ -0,0 +1,115 @@ +#include "TestHttpServer.h" + +#include +#include + +namespace PhpQml::Bridge::Tests { + +TestHttpServer::TestHttpServer(QObject* parent) + : QObject(parent) +{ + m_server.listen(QHostAddress::LocalHost, 0); + connect(&m_server, &QTcpServer::newConnection, + this, &TestHttpServer::onNewConnection); +} + +QString TestHttpServer::url() const +{ + return QStringLiteral("http://127.0.0.1:%1").arg(m_server.serverPort()); +} + +void TestHttpServer::setResponseBody(const QString& v) +{ + if (m_responseBody == v) return; + m_responseBody = v; + emit responseBodyChanged(); +} + +void TestHttpServer::setResponseStatus(int v) +{ + if (m_responseStatus == v) return; + m_responseStatus = v; + emit responseStatusChanged(); +} + +void TestHttpServer::onNewConnection() +{ + while (auto* sock = m_server.nextPendingConnection()) { + // One buffer per socket, owned by the socket so it dies with it. + // (The original thread_local trick leaked between connections.) + auto* buffer = new QByteArray; + connect(sock, &QObject::destroyed, [buffer]() { delete buffer; }); + + connect(sock, &QTcpSocket::readyRead, this, [this, sock, buffer]() { + buffer->append(sock->readAll()); + const int headerEnd = buffer->indexOf("\r\n\r\n"); + if (headerEnd < 0) return; + + const QByteArray headerBlock = buffer->left(headerEnd); + buffer->clear(); + + const QList lines = headerBlock.split('\n'); + QString requestLine; + QString authHeader; + for (int i = 0; i < lines.size(); ++i) { + QByteArray line = lines[i]; + if (line.endsWith('\r')) line.chop(1); + if (i == 0) { + requestLine = QString::fromUtf8(line); + continue; + } + const int colon = line.indexOf(':'); + if (colon < 0) continue; + const QByteArray name = line.left(colon).trimmed(); + const QByteArray value = line.mid(colon + 1).trimmed(); + if (name.compare("Authorization", Qt::CaseInsensitive) == 0) { + authHeader = QString::fromUtf8(value); + } + } + + // Only count + capture metrics for /api/… GETs. SSE reconnect + // attempts from MercureClient hit /.well-known/mercure on the + // same port and would otherwise inflate the request count and + // overwrite the captured headers we want to assert against. + const bool isApiGet = requestLine.startsWith(QStringLiteral("GET /api/")); + if (isApiGet) { + if (m_lastRequestLine != requestLine) { + m_lastRequestLine = requestLine; + emit lastRequestLineChanged(); + } + if (m_lastAuthHeader != authHeader) { + m_lastAuthHeader = authHeader; + emit lastAuthHeaderChanged(); + } + ++m_apiGetCount; + emit apiGetCountChanged(); + } + + // For /api/ routes, mimic SessionAuthenticator and reject + // requests without an Authorization header. This is what + // exposes the property-order race in the regression test: + // pre-fix, the GET went out unauthenticated, this server + // returned 401, and the model parked with `ready === false`. + const bool needAuth = isApiGet; + const bool isAuthed = !authHeader.isEmpty(); + const bool reject = needAuth && !isAuthed; + const int status = reject ? 401 : m_responseStatus; + const QByteArray body = reject + ? QByteArrayLiteral(R"({"type":"about:blank","title":"Unauthorized","status":401})") + : m_responseBody.toUtf8(); + + QByteArray resp; + resp.append("HTTP/1.1 ").append(QByteArray::number(status)) + .append(' ').append(status == 200 ? "OK" : "STATUS").append("\r\n"); + resp.append("Content-Type: application/json\r\n"); + resp.append("Content-Length: ").append(QByteArray::number(body.size())).append("\r\n"); + resp.append("Connection: close\r\n\r\n"); + resp.append(body); + sock->write(resp); + sock->disconnectFromHost(); + }); + connect(sock, &QTcpSocket::disconnected, sock, &QObject::deleteLater); + } +} + +} // namespace PhpQml::Bridge::Tests diff --git a/framework/qml/tests/TestHttpServer.h b/framework/qml/tests/TestHttpServer.h new file mode 100644 index 0000000..a64cee0 --- /dev/null +++ b/framework/qml/tests/TestHttpServer.h @@ -0,0 +1,76 @@ +// Tiny localhost HTTP server for qmltest fixtures. Listens on a free +// ephemeral port; for any incoming request, captures the request line + +// headers and replies with a fixed JSON body. Exposed to QML as the +// `TestHttpServer` element so tests can instantiate one inline: +// +// TestHttpServer { +// id: srv +// responseBody: '[{"id":"1","title":"a","done":false}]' +// } +// ReactiveListModel { baseUrl: srv.url; ... } +// compare(srv.lastAuthHeader, "Bearer testtoken") +// +// Just enough HTTP to serve a single line-of-sight request — no +// chunked encoding, no keepalive, no Content-Length parsing on the +// way in. The framework's network paths only ever issue GET /… +// against this stub during the test, so that's all we need. +// +// `apiGetCount` counts only requests under `/api/…` so tests can +// distinguish the model's HTTP fetches from Mercure's SSE reconnect +// attempts (which hit `/.well-known/mercure`). + +#pragma once + +#include +#include +#include + +namespace PhpQml::Bridge::Tests { + +class TestHttpServer : public QObject +{ + Q_OBJECT + QML_ELEMENT + + Q_PROPERTY(int port READ port CONSTANT) + Q_PROPERTY(QString url READ url CONSTANT) + Q_PROPERTY(QString responseBody READ responseBody WRITE setResponseBody NOTIFY responseBodyChanged) + Q_PROPERTY(int responseStatus READ responseStatus WRITE setResponseStatus NOTIFY responseStatusChanged) + Q_PROPERTY(int apiGetCount READ apiGetCount NOTIFY apiGetCountChanged) + Q_PROPERTY(QString lastAuthHeader READ lastAuthHeader NOTIFY lastAuthHeaderChanged) + Q_PROPERTY(QString lastRequestLine READ lastRequestLine NOTIFY lastRequestLineChanged) + +public: + explicit TestHttpServer(QObject* parent = nullptr); + + int port() const { return m_server.serverPort(); } + QString url() const; + QString responseBody() const { return m_responseBody; } + int responseStatus() const { return m_responseStatus; } + int apiGetCount() const { return m_apiGetCount; } + QString lastAuthHeader() const { return m_lastAuthHeader; } + QString lastRequestLine() const { return m_lastRequestLine; } + + void setResponseBody(const QString& v); + void setResponseStatus(int v); + +signals: + void responseBodyChanged(); + void responseStatusChanged(); + void apiGetCountChanged(); + void lastAuthHeaderChanged(); + void lastRequestLineChanged(); + +private slots: + void onNewConnection(); + +private: + QTcpServer m_server; + QString m_responseBody = QStringLiteral("[]"); + int m_responseStatus = 200; + int m_apiGetCount = 0; + QString m_lastAuthHeader; + QString m_lastRequestLine; +}; + +} // namespace PhpQml::Bridge::Tests diff --git a/framework/qml/tests/main.cpp b/framework/qml/tests/main.cpp index 428aca9..1de81b6 100644 --- a/framework/qml/tests/main.cpp +++ b/framework/qml/tests/main.cpp @@ -4,6 +4,18 @@ // // PLAN.md §13 v0.2.0 testing-strategy row. +#include #include +// Static QML modules need their auto-generated plugin classes pulled +// in explicitly — the linker would otherwise strip the registration +// init code because nothing in main() references it. Without these +// imports the QmlEngine that QUICK_TEST_MAIN spins up can't resolve +// `import PhpQml.Bridge` / `import PhpQml.Bridge.Tests`. +// +// Plugin class names are auto-generated by qt_add_qml_module(STATIC) +// from the URI: dots become underscores, suffixed with "Plugin". +Q_IMPORT_PLUGIN(PhpQml_BridgePlugin) +Q_IMPORT_PLUGIN(PhpQml_Bridge_TestsPlugin) + QUICK_TEST_MAIN(qml_unit_tests) diff --git a/framework/qml/tests/tst_reactive_list_model.qml b/framework/qml/tests/tst_reactive_list_model.qml new file mode 100644 index 0000000..76752cb --- /dev/null +++ b/framework/qml/tests/tst_reactive_list_model.qml @@ -0,0 +1,118 @@ +// Regression test for the property-assignment-order bug that left a +// second window's ReactiveListModel empty on first open. Both +// reproductions cover the same root cause: setBaseUrl() / setSource() +// used to fire `refresh()` inline, which meant whichever setter +// happened to land *last* triggered the GET — and that GET captured +// whatever m_token was at that exact instant. setToken() never fires +// refresh() itself, so if QML evaluated `token` after `baseUrl` / +// `source`, the first GET went out unauthenticated and the model +// parked an empty list. +// +// The fix defers the initial fetch to QQmlParserStatus::componentComplete(). +// By that point every binding (literal *and* singleton-derived) has +// landed, so refresh() picks up the bearer. + +import QtQuick +import QtTest +import PhpQml.Bridge +import PhpQml.Bridge.Tests + +TestCase { + name: "ReactiveListModel" + when: windowShown + + // ── Stub backend ──────────────────────────────────────────────── + TestHttpServer { + id: srv + responseBody: '[{"id":"1","title":"hello","done":false}]' + } + + // Stand-in for the BackendConnection singleton — exposes the same + // shape (`url`, `token` properties) so the model's bindings depend + // on a third object the same way the production code does. This is + // what reproduces the property-evaluation-order race: when both + // `baseUrl` and `token` are bindings (rather than literals), QML + // evaluates them together in the binding-evaluation phase, *after* + // the literal `source` has been assigned. Pre-fix, the binding + // for `baseUrl` fires `refresh()` inline and the request goes out + // before the binding for `token` has run. + QtObject { + id: backend + property string url: srv.url + property string token: "testtoken" + } + + // ── Reproduction A: declarative model with bindings up-front ──── + // This is the exact shape examples/todo/qml/TodoWindow.qml uses + // for the second window. Without the fix, the setter that lands + // *second* of {baseUrl, source} fires `refresh()` inline — and + // because QML evaluates literal values before bindings to other + // objects' properties, that setter typically lands before `token`. + // The GET goes out unauthenticated, the test server returns 401, + // and the model parks with `ready === false`. The fix defers the + // initial fetch to componentComplete() so the bearer is always in + // place by the time the request fires. + Component { + id: declarativeModel + ReactiveListModel { + // Same shape as examples/todo/qml/TodoWindow.qml — literals + // for source/topic, bindings to a stand-in BackendConnection + // for baseUrl/token. Without the fix the GET fires before + // `token` lands and the test server's auth check rejects + // it; the model parks at ready === false. + source: "/api/todos" + topic: "app://model/todo" + baseUrl: backend.url + token: backend.token + } + } + + function test_declarative_creation_sends_token_on_first_get() { + const baseline = srv.apiGetCount + const m = declarativeModel.createObject(null) + verify(m, "model instance was created") + + // Wait for the GET to land. With the fix, the request fires + // exactly once after componentComplete with the bearer set. + tryCompare(srv, "apiGetCount", baseline + 1, 2000, + "ReactiveListModel issued exactly one /api/ GET") + compare(srv.lastAuthHeader, "Bearer testtoken", + "first GET carries the Authorization header — without the fix this is empty") + compare(srv.lastRequestLine, "GET /api/todos HTTP/1.1", + "request line addresses the configured source path") + + tryCompare(m, "ready", true, 2000) + + m.destroy() + } + + // ── Reproduction B: post-componentComplete imperative changes ─── + // Once the component is complete, individual setter changes still + // need to trigger refresh inline. This case verifies the fix + // doesn't accidentally suppress refresh forever — only during the + // initial property-assignment pass. + Component { + id: bareModel + ReactiveListModel {} + } + + function test_imperative_property_set_after_completion() { + // Each test reuses the same TestHttpServer instance; check the + // delta from a snapshot taken now. + const baseline = srv.apiGetCount + + const m = bareModel.createObject(null) + verify(m) + + m.token = "imperativeToken" + m.topic = "app://model/todo" + m.source = "/api/todos" + m.baseUrl = srv.url // last of the {baseUrl, source} pair → triggers fetch + + tryCompare(srv, "apiGetCount", baseline + 1, 2000) + compare(srv.lastAuthHeader, "Bearer imperativeToken", + "imperative setBaseUrl after token is set fetches with token") + + m.destroy() + } +}