Improve the API of QCoapRequest

 - Moved internally used methods to the private class.
 - Replaced the internally used protected constructor with a static
   method in the private class.
 - Removed the internally used setMethod(), no need for it anymore.
 - Removed the isValid() method, no point in keeping it.

This change is based on the feedback from API review.

Change-Id: I177efdb1d436266549dea3e8d2b01160648fce90
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

10 files changed, 82 insertions, 101 deletions

diff --git a/src/coap/qcoapclient.cpp b/src/coap/qcoapclient.cpp
index 6e1e52b..af09686 100644
--- a/src/coap/qcoapclient.cpp
+++ b/src/coap/qcoapclient.cpp
@@ -35,6 +35,7 @@
 #include "qcoapnamespace.h"
 #include "qcoapsecurityconfiguration.h"
 #include "qcoapqudpconnection_p.h"
+#include "qcoaprequest_p.h"
 #include <QtCore/qiodevice.h>
 #include <QtCore/qurl.h>
 #include <QtCore/qloggingcategory.h>
@@ -227,9 +228,8 @@ QCoapReply *QCoapClient::get(const QCoapRequest &request)
 {
     Q_D(QCoapClient);
 
-    QCoapRequest copyRequest(request, QtCoap::Method::Get);
-    copyRequest.adjustUrl(d->connection->isSecure());
-
+    QCoapRequest copyRequest = QCoapRequestPrivate::createRequest(request, QtCoap::Method::Get,
+                                                                  d->connection->isSecure());
     return d->sendRequest(copyRequest);
 }
 
@@ -256,10 +256,9 @@ QCoapReply *QCoapClient::put(const QCoapRequest &request, const QByteArray &data
 {
     Q_D(QCoapClient);
 
-    QCoapRequest copyRequest(request, QtCoap::Method::Put);
+    QCoapRequest copyRequest = QCoapRequestPrivate::createRequest(request, QtCoap::Method::Put,
+                                                                  d->connection->isSecure());
     copyRequest.setPayload(data);
-    copyRequest.adjustUrl(d->connection->isSecure());
-
     return d->sendRequest(copyRequest);
 }
 
@@ -302,10 +301,9 @@ QCoapReply *QCoapClient::post(const QCoapRequest &request, const QByteArray &dat
 {
     Q_D(QCoapClient);
 
-    QCoapRequest copyRequest(request, QtCoap::Method::Post);
+    QCoapRequest copyRequest = QCoapRequestPrivate::createRequest(request, QtCoap::Method::Post,
+                                                                  d->connection->isSecure());
     copyRequest.setPayload(data);
-    copyRequest.adjustUrl(d->connection->isSecure());
-
     return d->sendRequest(copyRequest);
 }
 
@@ -351,9 +349,8 @@ QCoapReply *QCoapClient::deleteResource(const QCoapRequest &request)
 {
     Q_D(QCoapClient);
 
-    QCoapRequest copyRequest(request, QtCoap::Method::Delete);
-    copyRequest.adjustUrl(d->connection->isSecure());
-
+    QCoapRequest copyRequest = QCoapRequestPrivate::createRequest(request, QtCoap::Method::Delete,
+                                                                  d->connection->isSecure());
     return d->sendRequest(copyRequest);
 }
 
@@ -407,9 +404,9 @@ QCoapResourceDiscoveryReply *QCoapClient::discover(QtCoap::MulticastGroup group,
     discoveryUrl.setPath(discoveryPath);
     discoveryUrl.setPort(port);
 
-    QCoapRequest request(discoveryUrl);
-    request.setMethod(QtCoap::Method::Get);
-    request.adjustUrl(d->connection->isSecure());
+    QCoapRequest request = QCoapRequestPrivate::createRequest(QCoapRequest(discoveryUrl),
+                                                              QtCoap::Method::Get,
+                                                              d->connection->isSecure());
 
     return d->sendDiscovery(request);
 }
@@ -433,10 +430,9 @@ QCoapResourceDiscoveryReply *QCoapClient::discover(const QUrl &url, const QStrin
     QUrl discoveryUrl(url);
     discoveryUrl.setPath(url.path() + discoveryPath);
 
-    QCoapRequest request(discoveryUrl);
-    request.setMethod(QtCoap::Method::Get);
-    request.adjustUrl(d->connection->isSecure());
-
+    QCoapRequest request = QCoapRequestPrivate::createRequest(QCoapRequest(discoveryUrl),
+                                                              QtCoap::Method::Get,
+                                                              d->connection->isSecure());
     return d->sendDiscovery(request);
 }
 
@@ -449,7 +445,7 @@ QCoapResourceDiscoveryReply *QCoapClient::discover(const QUrl &url, const QStrin
 */
 QCoapReply *QCoapClient::observe(const QCoapRequest &request)
 {
-    QCoapRequest copyRequest(request, QtCoap::Method::Get);
+    QCoapRequest copyRequest = QCoapRequestPrivate::createRequest(request, QtCoap::Method::Get);
     copyRequest.enableObserve();
 
     return get(copyRequest);
@@ -494,7 +490,7 @@ void QCoapClient::cancelObserve(QCoapReply *notifiedReply)
 void QCoapClient::cancelObserve(const QUrl &url)
 {
     Q_D(QCoapClient);
-    const auto adjustedUrl = QCoapRequest::adjustedUrl(url, d->connection->isSecure());
+    const auto adjustedUrl = QCoapRequestPrivate::adjustedUrl(url, d->connection->isSecure());
     QMetaObject::invokeMethod(d->protocol, "cancelObserve", Q_ARG(QUrl, adjustedUrl));
 }
 
@@ -562,7 +558,7 @@ bool QCoapClientPrivate::send(QCoapReply *reply)
         return false;
     }
 
-    if (!QCoapRequest::isUrlValid(reply->request().url())) {
+    if (!QCoapRequestPrivate::isUrlValid(reply->request().url())) {
         qCWarning(lcCoapClient, "Failed to send request for an invalid URL.");
         return false;
     }
diff --git a/src/coap/qcoapinternalmessage.cpp b/src/coap/qcoapinternalmessage.cpp
index 85c2263..4da627e 100644
--- a/src/coap/qcoapinternalmessage.cpp
+++ b/src/coap/qcoapinternalmessage.cpp
@@ -29,6 +29,7 @@
 ****************************************************************************/
 
 #include "qcoapinternalmessage_p.h"
+#include "qcoaprequest_p.h"
 #include <QtCoap/qcoaprequest.h>
 
 #include <QtCore/qloggingcategory.h>
@@ -261,7 +262,7 @@ bool QCoapInternalMessage::isValid() const
 */
 bool QCoapInternalMessage::isUrlValid(const QUrl &url)
 {
-    return QCoapRequest::isUrlValid(url);
+    return QCoapRequestPrivate::isUrlValid(url);
 }
 
 QT_END_NAMESPACE
diff --git a/src/coap/qcoapprotocol.cpp b/src/coap/qcoapprotocol.cpp
index 2f4c278..28ee7e8 100644
--- a/src/coap/qcoapprotocol.cpp
+++ b/src/coap/qcoapprotocol.cpp
@@ -31,6 +31,7 @@
 #include "qcoapprotocol_p.h"
 #include "qcoapinternalrequest_p.h"
 #include "qcoapinternalreply_p.h"
+#include "qcoaprequest_p.h"
 #include "qcoapconnection_p.h"
 #include "qcoapnamespace_p.h"
 
@@ -134,7 +135,8 @@ void QCoapProtocol::sendRequest(QPointer<QCoapReply> reply, QCoapConnection *con
     Q_D(QCoapProtocol);
     Q_ASSERT(QThread::currentThread() == thread());
 
-    if (reply.isNull() || !reply->request().isValid())
+    if (reply.isNull() || reply->request().method() == QtCoap::Method::Invalid
+            || !QCoapRequestPrivate::isUrlValid(reply->request().url()))
         return;
 
     connect(reply, &QCoapReply::aborted, this, [this](const QCoapToken &token) {
diff --git a/src/coap/qcoaprequest.cpp b/src/coap/qcoaprequest.cpp
index 59cdb59..4a61643 100644
--- a/src/coap/qcoaprequest.cpp
+++ b/src/coap/qcoaprequest.cpp
@@ -138,19 +138,6 @@ QCoapRequest::QCoapRequest(const QCoapRequest &other) :
 }
 
 /*!
-    \internal
-
-    Constructs a copy of the \a other QCoapRequest and sets the request
-    method to \a method.
-*/
-QCoapRequest::QCoapRequest(const QCoapRequest &other, QtCoap::Method method) :
-    QCoapRequest(other)
-{
-    if (method != QtCoap::Method::Invalid)
-        setMethod(method);
-}
-
-/*!
     Destroys the QCoapRequest.
 */
 QCoapRequest::~QCoapRequest()
@@ -225,19 +212,6 @@ void QCoapRequest::setProxyUrl(const QUrl &proxyUrl)
 }
 
 /*!
-    \internal
-
-    Sets the method of the request to the given \a method.
-
-    \sa method()
-*/
-void QCoapRequest::setMethod(QtCoap::Method method)
-{
-    Q_D(QCoapRequest);
-    d->method = method;
-}
-
-/*!
     Sets the observe to \c true to make an observe request.
 
     \sa isObserve()
@@ -251,6 +225,8 @@ void QCoapRequest::enableObserve()
 }
 
 /*!
+    \internal
+
     Adjusts the request URL by setting the correct default scheme and port
     (if not indicated) based on the \a secure parameter.
 
@@ -258,10 +234,9 @@ void QCoapRequest::enableObserve()
     its port will default to \e 5683. In secure mode the scheme will default to
     \c coaps, and the port will default to \e 5684.
 */
-void QCoapRequest::adjustUrl(bool secure)
+void QCoapRequestPrivate::adjustUrl(bool secure)
 {
-    Q_D(QCoapRequest);
-    d->uri = adjustedUrl(d->uri, secure);
+    uri = adjustedUrl(uri, secure);
 }
 
 /*!
@@ -274,17 +249,11 @@ QCoapRequest &QCoapRequest::operator=(const QCoapRequest &other)
 }
 
 /*!
-    Returns \c true if the request is valid, \c false otherwise.
-*/
-bool QCoapRequest::isValid() const
-{
-    return isUrlValid(url()) && method() != QtCoap::Method::Invalid;
-}
+    \internal
 
-/*!
     Returns \c true if the \a url is a valid CoAP URL.
 */
-bool QCoapRequest::isUrlValid(const QUrl &url)
+bool QCoapRequestPrivate::isUrlValid(const QUrl &url)
 {
     return (url.isValid() && !url.isLocalFile() && !url.isRelative()
             && (url.scheme() == CoapScheme || url.scheme() == CoapSecureScheme)
@@ -292,6 +261,8 @@ bool QCoapRequest::isUrlValid(const QUrl &url)
 }
 
 /*!
+    \internal
+
     Adjusts the \a url by setting the correct default scheme and port
     (if not indicated) based on the \a secure parameter. Returns the
     adjusted URL.
@@ -300,7 +271,7 @@ bool QCoapRequest::isUrlValid(const QUrl &url)
     its port will default to \e 5683. In secure mode the scheme will default to
     \c coaps, and the port will default to \e 5684.
 */
-QUrl QCoapRequest::adjustedUrl(const QUrl &url, bool secure)
+QUrl QCoapRequestPrivate::adjustedUrl(const QUrl &url, bool secure)
 {
     if (url.isEmpty() || !url.isValid())
         return QUrl();
@@ -338,4 +309,19 @@ QCoapRequestPrivate* QCoapRequest::d_func()
     return static_cast<QCoapRequestPrivate*>(d_ptr.data());
 }
 
+/*!
+    \internal
+
+    Creates a copy of \a other request and sets \a method as its request method.
+    Adjusts the request URL based on \a isSecure parameter.
+*/
+QCoapRequest
+QCoapRequestPrivate::createRequest(const QCoapRequest &other, QtCoap::Method method, bool isSecure)
+{
+    QCoapRequest request(other);
+    request.d_func()->method = method;
+    request.d_func()->adjustUrl(isSecure);
+    return request;
+}
+
 QT_END_NAMESPACE
diff --git a/src/coap/qcoaprequest.h b/src/coap/qcoaprequest.h
index e506c83..848d357 100644
--- a/src/coap/qcoaprequest.h
+++ b/src/coap/qcoaprequest.h
@@ -61,16 +61,6 @@ public:
     void setUrl(const QUrl &url);
     void setProxyUrl(const QUrl &proxyUrl);
     void enableObserve();
-    void adjustUrl(bool secure);
-
-    bool isValid() const;
-    static bool isUrlValid(const QUrl &url);
-    static QUrl adjustedUrl(const QUrl &url, bool secure);
-
-protected:
-    QCoapRequest(const QCoapRequest &other, QtCoap::Method method);
-
-    void setMethod(QtCoap::Method method);
 
 private:
     // Q_DECLARE_PRIVATE equivalent for shared data pointers
@@ -78,7 +68,7 @@ private:
     const QCoapRequestPrivate* d_func() const
     { return reinterpret_cast<const QCoapRequestPrivate*>(d_ptr.constData()); }
 
-    friend class QCoapClient;
+    friend class QCoapRequestPrivate;
 };
 
 QT_END_NAMESPACE
diff --git a/src/coap/qcoaprequest_p.h b/src/coap/qcoaprequest_p.h
index eae236d..20af324 100644
--- a/src/coap/qcoaprequest_p.h
+++ b/src/coap/qcoaprequest_p.h
@@ -58,6 +58,12 @@ public:
     ~QCoapRequestPrivate();
 
     void setUrl(const QUrl &url);
+    void adjustUrl(bool secure);
+
+    static QCoapRequest createRequest(const QCoapRequest &other, QtCoap::Method method,
+                                      bool isSecure = false);
+    static QUrl adjustedUrl(const QUrl &url, bool secure);
+    static bool isUrlValid(const QUrl &url);
 
     QUrl uri;
     QUrl proxyUri;
diff --git a/tests/auto/qcoapclient/tst_qcoapclient.cpp b/tests/auto/qcoapclient/tst_qcoapclient.cpp
index c7fce74..2b914ab 100644
--- a/tests/auto/qcoapclient/tst_qcoapclient.cpp
+++ b/tests/auto/qcoapclient/tst_qcoapclient.cpp
@@ -40,6 +40,7 @@
 #include <private/qcoapclient_p.h>
 #include <private/qcoapqudpconnection_p.h>
 #include <private/qcoapprotocol_p.h>
+#include <private/qcoaprequest_p.h>
 
 #include "../coapnetworksettings.h"
 
@@ -273,7 +274,9 @@ void tst_QCoapClient::methods()
     }
 
     QVERIFY2(!reply.isNull(), "Request failed unexpectedly");
-    QCOMPARE(reply->url(), QCoapRequest::adjustedUrl(url, false));
+#ifdef QT_BUILD_INTERNAL
+    QCOMPARE(reply->url(), QCoapRequestPrivate::adjustedUrl(url, false));
+#endif
     QSignalSpy spyReplyFinished(reply.data(), SIGNAL(finished(QCoapReply *)));
     QTRY_COMPARE(spyReplyFinished.count(), 1);
     QTRY_COMPARE(spyClientFinished.count(), 1);
@@ -705,7 +708,9 @@ void tst_QCoapClient::discover()
     QTRY_COMPARE_WITH_TIMEOUT(spyReplyFinished.count(), 1, 30000);
 
     const auto discoverUrl = QUrl(url.toString() + "/.well-known/core");
-    QCOMPARE(resourcesReply->url(), QCoapRequest::adjustedUrl(discoverUrl, false));
+#ifdef QT_BUILD_INTERNAL
+    QCOMPARE(resourcesReply->url(), QCoapRequestPrivate::adjustedUrl(discoverUrl, false));
+#endif
     QCOMPARE(resourcesReply->resources().length(), resourceNumber);
     QCOMPARE(resourcesReply->request().method(), QtCoap::Method::Get);
 
@@ -771,7 +776,9 @@ void tst_QCoapClient::observe()
 
     QTRY_COMPARE_WITH_TIMEOUT(spyReplyNotified.count(), 3, 30000);
     client.cancelObserve(reply.data());
-    QCOMPARE(reply->url(), QCoapRequest::adjustedUrl(url, false));
+#ifdef QT_BUILD_INTERNAL
+    QCOMPARE(reply->url(), QCoapRequestPrivate::adjustedUrl(url, false));
+#endif
     QCOMPARE(reply->request().method(), QtCoap::Method::Get);
 
     QVERIFY2(!spyReplyNotified.wait(7000), "'Notify' signal received after cancelling observe");
diff --git a/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp b/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp
index 1e5c732..1a1a293 100644
--- a/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp
+++ b/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp
@@ -33,6 +33,7 @@
 
 #include <QtCoap/qcoaprequest.h>
 #include <private/qcoapinternalrequest_p.h>
+#include <private/qcoaprequest_p.h>
 
 #ifdef QT_BUILD_INTERNAL
 
@@ -53,12 +54,6 @@ private Q_SLOTS:
     void isMulticast();
 };
 
-struct QCoapRequestForTest : public QCoapRequest
-{
-    QCoapRequestForTest(const QUrl& url) : QCoapRequest(url) {}
-    using QCoapRequest::setMethod;
-};
-
 void tst_QCoapInternalRequest::requestToFrame_data()
 {
     QTest::addColumn<QUrl>("url");
@@ -153,9 +148,8 @@ void tst_QCoapInternalRequest::requestToFrame()
     QFETCH(QString, pduHeader);
     QFETCH(QString, pduPayload);
 
-    QCoapRequestForTest request(url);
+    QCoapRequest request = QCoapRequestPrivate::createRequest(QCoapRequest(url), method);
     request.setType(type);
-    request.setMethod(method);
     request.setPayload(pduPayload.toUtf8());
     request.setMessageId(messageId);
     request.setToken(token);
diff --git a/tests/auto/qcoapqudpconnection/tst_qcoapqudpconnection.cpp b/tests/auto/qcoapqudpconnection/tst_qcoapqudpconnection.cpp
index 947cb5d..ee8c295 100644
--- a/tests/auto/qcoapqudpconnection/tst_qcoapqudpconnection.cpp
+++ b/tests/auto/qcoapqudpconnection/tst_qcoapqudpconnection.cpp
@@ -40,18 +40,13 @@
 #include <QtCoap/qcoaprequest.h>
 #include <private/qcoapqudpconnection_p.h>
 #include <private/qcoapinternalrequest_p.h>
+#include <private/qcoaprequest_p.h>
 #include "../coapnetworksettings.h"
 
 #ifdef QT_BUILD_INTERNAL
 
 using namespace QtCoapNetworkSettings;
 
-struct QCoapRequestForTest : public QCoapRequest
-{
-    QCoapRequestForTest(const QUrl &url) : QCoapRequest(url) {}
-    using QCoapRequest::setMethod;
-};
-
 class tst_QCoapQUdpConnection : public QObject
 {
     Q_OBJECT
@@ -186,10 +181,10 @@ void tst_QCoapQUdpConnection::sendRequest()
     QSignalSpy spySocketReadyRead(connection.socket(), &QUdpSocket::readyRead);
     QSignalSpy spyConnectionReadyRead(&connection, &QCoapQUdpConnection::readyRead);
 
-    QCoapRequestForTest request(protocol + host + path);
+    QCoapRequest request =
+            QCoapRequestPrivate::createRequest(QCoapRequest(protocol + host + path), method);
     request.setMessageId(24806);
     request.setToken(QByteArray("abcd"));
-    request.setMethod(method);
     QVERIFY(connection.socket() != nullptr);
     QCoapInternalRequest internalRequest(request);
     connection.sendRequest(internalRequest.toQByteArray(), host, port);
diff --git a/tests/auto/qcoaprequest/tst_qcoaprequest.cpp b/tests/auto/qcoaprequest/tst_qcoaprequest.cpp
index 592d4eb..2b042b5 100644
--- a/tests/auto/qcoaprequest/tst_qcoaprequest.cpp
+++ b/tests/auto/qcoaprequest/tst_qcoaprequest.cpp
@@ -34,6 +34,7 @@
 #include <QtCoap/qcoapglobal.h>
 #include <QtCoap/qcoapnamespace.h>
 #include <QtCoap/qcoaprequest.h>
+#include <private/qcoaprequest_p.h>
 
 class tst_QCoapRequest : public QObject
 {
@@ -50,11 +51,6 @@ private Q_SLOTS:
     void copyAndDetach();
 };
 
-struct QCoapRequestForTest : public QCoapRequest
-{
-    using QCoapRequest::setMethod;
-};
-
 void tst_QCoapRequest::ctor_data()
 {
     QTest::addColumn<QUrl>("url");
@@ -103,13 +99,18 @@ void tst_QCoapRequest::adjustUrl_data()
 
 void tst_QCoapRequest::adjustUrl()
 {
+#ifdef QT_BUILD_INTERNAL
     QFETCH(QUrl, inputUrl);
     QFETCH(QUrl, expectedUrl);
     QFETCH(bool, secure);
 
-    QCoapRequest request(inputUrl);
-    request.adjustUrl(secure);
+    // createRequest() will adjust the url
+    QCoapRequest request = QCoapRequestPrivate::createRequest(QCoapRequest(inputUrl),
+                                                              QtCoap::Method::Get, secure);
     QCOMPARE(request.url(), expectedUrl);
+#else
+    QSKIP("Not an internal build, skipping this test");
+#endif
 }
 
 void tst_QCoapRequest::setUrl_data()
@@ -156,13 +157,13 @@ void tst_QCoapRequest::enableObserve()
 
 void tst_QCoapRequest::copyAndDetach()
 {
-    QCoapRequestForTest a;
+#ifdef QT_BUILD_INTERNAL
+    QCoapRequest a = QCoapRequestPrivate::createRequest(QCoapRequest(), QtCoap::Method::Delete);
     a.setMessageId(3);
     a.setPayload("payload");
     a.setToken("token");
     a.setType(QCoapMessage::Type::Acknowledgment);
     a.setVersion(5);
-    a.setMethod(QtCoap::Method::Delete);
     QUrl testUrl("coap://url:500/resource");
     a.setUrl(testUrl);
     QUrl testProxyUrl("test://proxyurl");
@@ -186,6 +187,9 @@ void tst_QCoapRequest::copyAndDetach()
     c.setMessageId(9);
     QCOMPARE(c.messageId(), 9);
     QCOMPARE(a.messageId(), 3);
+#else
+    QSKIP("Not an internal build, skipping this test");
+#endif
 }
 
 QTEST_APPLESS_MAIN(tst_QCoapRequest)