Skip to content

Commit

Permalink
Http: fix how we treat must-revalidate
Browse files Browse the repository at this point in the history 
We calculated the expiry at time of download by looking at Max-Age, but
as long as must-revalidate was true we would just perform the network
request anyway.

One issue this, further, highlights is our PreferNetwork and PreferCache
options having no behavioral differences while being documented to
do different things. They both consult the cache first and returns any
non-stale data from there.

Fixes: QTBUG-128484
Pick-to: 6.8
Change-Id: I24b62e2bb072446e463482a778da7cfbd82a6835
Reviewed-by: Mate Barany <[email protected]>
  • Loading branch information
@Morten242
Morten242 committed Oct 31, 2024
1 parent 070d21a commit e4a35df
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 6 deletions.
2 changes: 0 additions & 2 deletions src/network/access/qnetworkreplyhttpimpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -531,8 +531,6 @@ bool QNetworkReplyHttpImplPrivate::loadFromCacheIfAllowed(QHttpNetworkRequest &h
value = cacheHeaders.value(QHttpHeaders::WellKnownHeader::CacheControl);
if (!value.empty()) {
QHash<QByteArray, QByteArray> cacheControl = parseHttpOptionHeader(value);
if (cacheControl.contains("must-revalidate"_ba))
return false;
if (cacheControl.contains("no-cache"_ba))
return false;
}
Expand Down
44 changes: 40 additions & 4 deletions tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4602,24 +4602,52 @@ void tst_QNetworkReply::ioGetFromHttpWithCache_data()
// Set must-revalidate now
//
rawHeaders.clear();
content.first.setExpirationDate(future);
rawHeaders << QNetworkCacheMetaData::RawHeader("Date", QLocale::c().toString(past, dateFormat).toLatin1())
<< QNetworkCacheMetaData::RawHeader("Cache-control", "max-age=7200, must-revalidate"); // must-revalidate is used
<< QNetworkCacheMetaData::RawHeader("Cache-control", "max-age=3600, must-revalidate"); // must-revalidate is used
content.first.setRawHeaders(rawHeaders);

QTest::newRow("must-revalidate,200,always-network")
<< reply200 << "Reloaded" << content << int(QNetworkRequest::AlwaysNetwork) << QStringList() << false << true;
QTest::newRow("must-revalidate,200,prefer-network")
<< reply200 << "Reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << false << true;
QTest::newRow("must-revalidate,200,prefer-cache")
<< reply200 << "Reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << false << true;
<< reply200 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << false;
QTest::newRow("must-revalidate,200,always-cache")
<< reply200 << "" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << false << false;
<< reply200 << "Not-reloaded" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << true << false;

QTest::newRow("must-revalidate,304,prefer-network")
<< reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << true << true;
QTest::newRow("must-revalidate,304,prefer-cache")
<< reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << true;
<< reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << false;
QTest::newRow("must-revalidate,304,always-cache")
<< reply304 << "Not-reloaded" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << true << false;

//
// Set must-revalidate, but also add Age, meaning a 3rd party (i.e. proxy) held on to the
// response for some time
//
rawHeaders.clear();
content.first.setExpirationDate(past);
rawHeaders << QNetworkCacheMetaData::RawHeader("Cache-control", "max-age=3600, must-revalidate")
// Some 3rd party held on to the response long enough that it expired:
<< QNetworkCacheMetaData::RawHeader("Age", "3600");
content.first.setRawHeaders(rawHeaders);

QTest::newRow("must-revalidate,200,always-network,expired")
<< reply200 << "Reloaded" << content << int(QNetworkRequest::AlwaysNetwork) << QStringList() << false << true;
QTest::newRow("must-revalidate,200,prefer-network,expired")
<< reply200 << "Reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << false << true;
QTest::newRow("must-revalidate,200,prefer-cache,expired")
<< reply200 << "Reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << false << true;
QTest::newRow("must-revalidate,200,always-cache,expired")
<< reply200 << "" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << false << false;

QTest::newRow("must-revalidate,304,prefer-network,expired")
<< reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << true << true;
QTest::newRow("must-revalidate,304,prefer-cache,expired")
<< reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << true;
QTest::newRow("must-revalidate,304,always-cache,expired")
<< reply304 << "" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << false << false;

//
Expand Down Expand Up @@ -4677,7 +4705,15 @@ void tst_QNetworkReply::ioGetFromHttpWithCache()

QVERIFY(waitForFinish(reply) != Timeout);

QEXPECT_FAIL("must-revalidate,200,always-cache",
"We assume the response is stale even though it is not", Abort);
QEXPECT_FAIL("must-revalidate,304,always-cache",
"We assume the response is stale even though it is not", Abort);
QEXPECT_FAIL("must-revalidate,200,prefer-network",
"PreferNetwork doesn't prefer network", Abort);
QTEST(reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool(), "loadedFromCache");
QEXPECT_FAIL("must-revalidate,304,prefer-network",
"PreferNetwork doesn't prefer network", Abort);
QTEST(server.totalConnections > 0, "networkUsed");
QFETCH(QString, body);
QCOMPARE(reply->readAll().constData(), qPrintable(body));
Expand Down

0 comments on commit e4a35df

Please sign in to comment.