Improve the API of QCoapOption

 - The RFC specifies the following option formats: empty, opaque, uint
   and string. Leave only the construcors for the specified types.
 - Rename value getters to reflect the expected value type.
 - Move the internally used setter methods to the private class.

This change is based on the feedback from API review.

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

10 files changed, 102 insertions, 124 deletions

diff --git a/src/coap/qcoapinternalmessage.cpp b/src/coap/qcoapinternalmessage.cpp
index 4da627e..121ee74 100644
--- a/src/coap/qcoapinternalmessage.cpp
+++ b/src/coap/qcoapinternalmessage.cpp
@@ -124,7 +124,7 @@ void QCoapInternalMessage::setFromDescriptiveBlockOption(const QCoapOption &opti
     Q_D(QCoapInternalMessage);
 
     //! TODO Cover with tests
-    const quint8 *optionData = reinterpret_cast<const quint8 *>(option.value().data());
+    const quint8 *optionData = reinterpret_cast<const quint8 *>(option.opaqueValue().data());
     const quint8 lastByte = optionData[option.length() - 1];
     quint32 blockNumber = 0;
 
diff --git a/src/coap/qcoapinternalreply.cpp b/src/coap/qcoapinternalreply.cpp
index fe27ee0..2480a05 100644
--- a/src/coap/qcoapinternalreply.cpp
+++ b/src/coap/qcoapinternalreply.cpp
@@ -188,7 +188,7 @@ int QCoapInternalReply::nextBlockToSend() const
     if (!option.isValid())
         return -1;
 
-    const quint8 *optionData = reinterpret_cast<const quint8 *>(option.value().data());
+    const quint8 *optionData = reinterpret_cast<const quint8 *>(option.opaqueValue().data());
     const quint8 lastByte = optionData[option.length() - 1];
 
     // M field
diff --git a/src/coap/qcoapinternalrequest.cpp b/src/coap/qcoapinternalrequest.cpp
index 0237259..ff6354c 100644
--- a/src/coap/qcoapinternalrequest.cpp
+++ b/src/coap/qcoapinternalrequest.cpp
@@ -231,7 +231,7 @@ QByteArray QCoapInternalRequest::toQByteArray() const
             if (isOptionLengthExtended)
                 appendByte(&pdu, optionLengthExtended);
 
-            pdu.append(option.value());
+            pdu.append(option.opaqueValue());
 
             lastOptionNumber = option.name();
         }
diff --git a/src/coap/qcoapoption.cpp b/src/coap/qcoapoption.cpp
index ddb42fd..4973091 100644
--- a/src/coap/qcoapoption.cpp
+++ b/src/coap/qcoapoption.cpp
@@ -88,59 +88,45 @@ Q_LOGGING_CATEGORY(lcCoapOption, "qt.coap.option")
 
 /*!
     Constructs a new CoAP option with the given \a name
-    and QByteArray \a value.
+    and QByteArray \a opaqueValue.
     If no parameters are passed, constructs an Invalid object.
 
     \sa isValid()
  */
-QCoapOption::QCoapOption(OptionName name, const QByteArray &value) :
+QCoapOption::QCoapOption(OptionName name, const QByteArray &opaqueValue) :
     d_ptr(new QCoapOptionPrivate)
 {
     Q_D(QCoapOption);
     d->name = name;
-    setValue(value);
+    d->setValue(opaqueValue);
 }
 
 /*!
     Constructs a new CoAP option with the given \a name
-    and the QStringView \a value.
+    and the QString \a stringValue.
 
     \sa isValid()
  */
-QCoapOption::QCoapOption(OptionName name, QStringView value) :
+QCoapOption::QCoapOption(OptionName name, const QString &stringValue) :
     d_ptr(new QCoapOptionPrivate)
 {
     Q_D(QCoapOption);
     d->name = name;
-    setValue(value);
+    d->setValue(stringValue);
 }
 
 /*!
     Constructs a new CoAP option with the given \a name
-    and the string \a value.
+    and the unsigned integer \a intValue.
 
     \sa isValid()
  */
-QCoapOption::QCoapOption(OptionName name, const char *value) :
+QCoapOption::QCoapOption(OptionName name, quint32 intValue) :
     d_ptr(new QCoapOptionPrivate)
 {
     Q_D(QCoapOption);
     d->name = name;
-    setValue(value);
-}
-
-/*!
-    Constructs a new CoAP option with the given \a name
-    and the unsigned integer \a value.
-
-    \sa isValid()
- */
-QCoapOption::QCoapOption(OptionName name, quint32 value) :
-    d_ptr(new QCoapOptionPrivate)
-{
-    Q_D(QCoapOption);
-    d->name = name;
-    setValue(value);
+    d->setValue(intValue);
 }
 
 /*!
@@ -203,7 +189,7 @@ void QCoapOption::swap(QCoapOption &other) Q_DECL_NOTHROW
 /*!
     Returns the value of the option.
  */
-QByteArray QCoapOption::value() const
+QByteArray QCoapOption::opaqueValue() const
 {
     Q_D(const QCoapOption);
     return d->value;
@@ -212,7 +198,7 @@ QByteArray QCoapOption::value() const
 /*!
     Returns the integer value of the option.
  */
-quint32 QCoapOption::valueToInt() const
+quint32 QCoapOption::uintValue() const
 {
     Q_D(const QCoapOption);
 
@@ -224,6 +210,15 @@ quint32 QCoapOption::valueToInt() const
 }
 
 /*!
+    Returns the QString value of the option.
+*/
+QString QCoapOption::stringValue() const
+{
+    Q_D(const QCoapOption);
+    return QString::fromUtf8(d->value);
+}
+
+/*!
     Returns the length of the value of the option.
  */
 int QCoapOption::length() const
@@ -269,95 +264,88 @@ bool QCoapOption::operator!=(const QCoapOption &other) const
 }
 
 /*!
+    \internal
+
     Sets the \a value for the option.
  */
-void QCoapOption::setValue(const QByteArray &value)
+void QCoapOptionPrivate::setValue(const QByteArray &opaqueValue)
 {
-    Q_D(QCoapOption);
     bool oversized = false;
 
     // Check for value maximum size, according to section 5.10 of RFC 7252
     // https://tools.ietf.org/html/rfc7252#section-5.10
-    switch (d_ptr->name) {
-    case IfNoneMatch:
-        if (value.size() > 0)
+    switch (name) {
+    case QCoapOption::IfNoneMatch:
+        if (opaqueValue.size() > 0)
             oversized = true;
         break;
 
-    case UriPort:
-    case ContentFormat:
-    case Accept:
-        if (value.size() > 2)
+    case QCoapOption::UriPort:
+    case QCoapOption::ContentFormat:
+    case QCoapOption::Accept:
+        if (opaqueValue.size() > 2)
             oversized = true;
         break;
 
-    case MaxAge:
-    case Size1:
-        if (value.size() > 4)
+    case QCoapOption::MaxAge:
+    case QCoapOption::Size1:
+        if (opaqueValue.size() > 4)
             oversized = true;
         break;
 
-    case IfMatch:
-    case Etag:
-        if (value.size() > 8)
+    case QCoapOption::IfMatch:
+    case QCoapOption::Etag:
+        if (opaqueValue.size() > 8)
             oversized = true;
         break;
 
-    case UriHost:
-    case LocationPath:
-    case UriPath:
-    case UriQuery:
-    case LocationQuery:
-    case ProxyScheme:
-        if (value.size() > 255)
+    case QCoapOption::UriHost:
+    case QCoapOption::LocationPath:
+    case QCoapOption::UriPath:
+    case QCoapOption::UriQuery:
+    case QCoapOption::LocationQuery:
+    case QCoapOption::ProxyScheme:
+        if (opaqueValue.size() > 255)
             oversized = true;
         break;
 
-    case ProxyUri:
-        if (value.size() > 1034)
+    case QCoapOption::ProxyUri:
+        if (opaqueValue.size() > 1034)
             oversized = true;
         break;
 
-    case Observe:
-    case Block2:
-    case Block1:
-    case Size2:
+    case QCoapOption::Observe:
+    case QCoapOption::Block2:
+    case QCoapOption::Block1:
+    case QCoapOption::Size2:
     default:
         break;
     }
 
     if (oversized)
-        qCWarning(lcCoapOption) << "Value" << value << "is probably too big for option" << d->name;
+        qCWarning(lcCoapOption) << "Value" << opaqueValue << "is probably too big for option" << name;
 
-    d->value = value;
+    value = opaqueValue;
 }
 
 /*!
+    \internal
     \overload
 
     Sets the \a value for the option.
  */
-void QCoapOption::setValue(QStringView value)
+void QCoapOptionPrivate::setValue(const QString &value)
 {
     setValue(value.toUtf8());
 }
 
 /*!
+    \internal
     \overload
 
     Sets the \a value for the option.
  */
-void QCoapOption::setValue(const char *value)
-{
-    setValue(QByteArray(value, static_cast<int>(strlen(value))));
-}
-
-/*!
-    \overload
-
-    Sets the \a value for the option.
- */
-void QCoapOption::setValue(quint32 value)
+void QCoapOptionPrivate::setValue(quint32 value)
 {
     QByteArray data;
     for (; value; value >>= 8)
diff --git a/src/coap/qcoapoption.h b/src/coap/qcoapoption.h
index ea9dc3c..ed8ebaf 100644
--- a/src/coap/qcoapoption.h
+++ b/src/coap/qcoapoption.h
@@ -64,10 +64,9 @@ public:
         Size1           = 60
     };
 
-    QCoapOption(OptionName name = Invalid, const QByteArray &value = QByteArray());
-    QCoapOption(OptionName name, QStringView value);
-    QCoapOption(OptionName name, const char *value);
-    QCoapOption(OptionName name, quint32 value);
+    QCoapOption(OptionName name = Invalid, const QByteArray &opaqueValue = QByteArray());
+    QCoapOption(OptionName name, const QString &stringValue);
+    QCoapOption(OptionName name, quint32 intValue);
     QCoapOption(const QCoapOption &other);
     QCoapOption(QCoapOption &&other);
     ~QCoapOption();
@@ -76,8 +75,9 @@ public:
     QCoapOption &operator=(QCoapOption &&other) Q_DECL_NOTHROW;
     void swap(QCoapOption &other) Q_DECL_NOTHROW;
 
-    QByteArray value() const;
-    quint32 valueToInt() const;
+    QByteArray opaqueValue() const;
+    quint32 uintValue() const;
+    QString stringValue() const;
     int length() const;
     OptionName name() const;
     bool isValid() const;
@@ -85,12 +85,6 @@ public:
     bool operator==(const QCoapOption &other) const;
     bool operator!=(const QCoapOption &other) const;
 
-protected:
-    void setValue(const QByteArray &value);
-    void setValue(QStringView value);
-    void setValue(const char *value);
-    void setValue(quint32 value);
-
 private:
     QCoapOptionPrivate *d_ptr;
 
diff --git a/src/coap/qcoapoption_p.h b/src/coap/qcoapoption_p.h
index 9cc1bac..ff60acd 100644
--- a/src/coap/qcoapoption_p.h
+++ b/src/coap/qcoapoption_p.h
@@ -52,6 +52,10 @@ class Q_AUTOTEST_EXPORT QCoapOptionPrivate
 public:
     QCoapOptionPrivate() = default;
 
+    void setValue(const QByteArray &opaqueValue);
+    void setValue(const QString &value);
+    void setValue(quint32 value);
+
     QCoapOption::OptionName name = QCoapOption::Invalid;
     QByteArray value;
 };
diff --git a/tests/auto/qcoapinternalreply/tst_qcoapinternalreply.cpp b/tests/auto/qcoapinternalreply/tst_qcoapinternalreply.cpp
index 7915f44..f9f46bd 100644
--- a/tests/auto/qcoapinternalreply/tst_qcoapinternalreply.cpp
+++ b/tests/auto/qcoapinternalreply/tst_qcoapinternalreply.cpp
@@ -160,7 +160,7 @@ void tst_QCoapInternalReply::parseReplyPdu()
         QCoapOption option = reply->message()->optionAt(i);
         QCOMPARE(option.name(), optionsNames.at(i));
         QCOMPARE(option.length(), optionsLengths.at(i));
-        QCOMPARE(option.value(), optionsValues.at(i));
+        QCOMPARE(option.opaqueValue(), optionsValues.at(i));
     }
     QCOMPARE(reply->message()->payload(), payload);
 }
diff --git a/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp b/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp
index 1a1a293..d3e367b 100644
--- a/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp
+++ b/tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp
@@ -177,33 +177,34 @@ void tst_QCoapInternalRequest::parseUri_data()
                         << QUrl()
                         << QVector<QCoapOption>({
                             QCoapOption(QCoapOption::UriPort, 1234),
-                            QCoapOption(QCoapOption::UriPath, "test"),
-                            QCoapOption(QCoapOption::UriPath, "path1") });
+                            QCoapOption(QCoapOption::UriPath, QByteArray("test")),
+                            QCoapOption(QCoapOption::UriPath, QByteArray("path1")) });
 
     QTest::newRow("path_query")
                         << QUrl("coap://10.20.30.40/test/path1/?rd=25&nd=4")
                         << QUrl()
                         << QVector<QCoapOption>({
-                            QCoapOption(QCoapOption::UriPath, "test"),
-                            QCoapOption(QCoapOption::UriPath, "path1"),
-                            QCoapOption(QCoapOption::UriQuery, "rd=25"),
-                            QCoapOption(QCoapOption::UriQuery, "nd=4") });
+                            QCoapOption(QCoapOption::UriPath, QByteArray("test")),
+                            QCoapOption(QCoapOption::UriPath, QByteArray("path1")),
+                            QCoapOption(QCoapOption::UriQuery, QByteArray("rd=25")),
+                            QCoapOption(QCoapOption::UriQuery, QByteArray("nd=4")) });
 
     QTest::newRow("host_path_query")
                         << QUrl("coap://aa.bb.cc.com:5683/test/path1/?rd=25&nd=4")
                         << QUrl()
                         << QVector<QCoapOption>({
-                            QCoapOption(QCoapOption::UriHost, "aa.bb.cc.com"),
-                            QCoapOption(QCoapOption::UriPath, "test"),
-                            QCoapOption(QCoapOption::UriPath, "path1"),
-                            QCoapOption(QCoapOption::UriQuery, "rd=25"),
-                            QCoapOption(QCoapOption::UriQuery, "nd=4") });
+                            QCoapOption(QCoapOption::UriHost, QByteArray("aa.bb.cc.com")),
+                            QCoapOption(QCoapOption::UriPath, QByteArray("test")),
+                            QCoapOption(QCoapOption::UriPath, QByteArray("path1")),
+                            QCoapOption(QCoapOption::UriQuery, QByteArray("rd=25")),
+                            QCoapOption(QCoapOption::UriQuery, QByteArray("nd=4")) });
 
     QTest::newRow("proxy_url")
                         << QUrl("coap://aa.bb.cc.com:5683/test/path1/?rd=25&nd=4")
                         << QUrl("coap://10.20.30.40/test:5684/othertest/path")
                         << QVector<QCoapOption>({
-                            QCoapOption(QCoapOption::ProxyUri, "coap://10.20.30.40/test:5684/othertest/path") });
+                            QCoapOption(QCoapOption::ProxyUri,
+                            QByteArray("coap://10.20.30.40/test:5684/othertest/path")) });
 }
 
 void tst_QCoapInternalRequest::parseUri()
@@ -227,9 +228,9 @@ void tst_QCoapInternalRequest::urlOptions_data()
     QTest::addColumn<QVector<QCoapOption>>("options");
 
     QVector<QCoapOption> options = {
-        { QCoapOption::UriHost, "example.com" },
-        { QCoapOption::UriPath, "~sensors" },
-        { QCoapOption::UriPath, "temp.xml" }
+        { QCoapOption::UriHost, QByteArray("example.com") },
+        { QCoapOption::UriPath, QByteArray("~sensors") },
+        { QCoapOption::UriPath, QByteArray("temp.xml") }
     };
 
     QTest::newRow("url_with_default_port")
diff --git a/tests/auto/qcoapmessage/tst_qcoapmessage.cpp b/tests/auto/qcoapmessage/tst_qcoapmessage.cpp
index e13281e..56579e5 100644
--- a/tests/auto/qcoapmessage/tst_qcoapmessage.cpp
+++ b/tests/auto/qcoapmessage/tst_qcoapmessage.cpp
@@ -123,7 +123,7 @@ void tst_QCoapMessage::addOption()
 
     QVERIFY(std::all_of(message.options().cbegin(), message.options().cend(),
                         [value](const QCoapOption opt) -> bool {
-                            return opt.value() == value;
+                            return opt.opaqueValue() == value;
            }));
 }
 
@@ -131,7 +131,7 @@ void tst_QCoapMessage::addOption_string_data()
 {
     QTest::addColumn<QVector<QCoapOption>>("options");
 
-    QVector<QCoapOption> single_char_option = { { QCoapOption::LocationPath, "path1" } };
+    QVector<QCoapOption> single_string_option = { { QCoapOption::LocationPath, QString("path1") } };
     QVector<QCoapOption> single_ba_option = {
         { QCoapOption::LocationPath, QByteArray("\xAF\x01\xC2") }
     };
@@ -140,7 +140,7 @@ void tst_QCoapMessage::addOption_string_data()
         { QCoapOption::LocationPath, QString("str_path3") }
     };
 
-    QTest::newRow("single_char_option") << single_char_option;
+    QTest::newRow("single_char_option") << single_string_option;
     QTest::newRow("single_ba_option") << single_ba_option;
     QTest::newRow("multiple_string_options") << multiple_string_options;
 }
@@ -184,18 +184,18 @@ void tst_QCoapMessage::addOption_uint()
     message.addOption(option);
 
     QCOMPARE(message.options(name).size(), 1);
-    QCOMPARE(message.option(name).valueToInt(), value);
-    QCOMPARE(option.value().size(), size);
+    QCOMPARE(message.option(name).uintValue(), value);
+    QCOMPARE(option.opaqueValue().size(), size);
 }
 
 void tst_QCoapMessage::removeOption_data()
 {
     QTest::addColumn<QVector<QCoapOption>>("options");
 
-    QVector<QCoapOption> single_option = { { QCoapOption::LocationPath, "path1" } };
+    QVector<QCoapOption> single_option = { { QCoapOption::LocationPath, QByteArray("path1") } };
     QVector<QCoapOption> multiple_options = {
-        { QCoapOption::LocationPath, "path2" },
-        { QCoapOption::LocationPath, "path3" }
+        { QCoapOption::LocationPath, QByteArray("path2") },
+        { QCoapOption::LocationPath, QByteArray("path3") }
     };
 
     QTest::newRow("single_option") << single_option;
@@ -227,10 +227,10 @@ void tst_QCoapMessage::removeOptionByName_data()
     QTest::addColumn<QVector<QCoapOption>>("options");
     QTest::addColumn<QCoapOption::OptionName>("name");
 
-    QVector<QCoapOption> single_option = { { QCoapOption::LocationPath, "path1" } };
+    QVector<QCoapOption> single_option = { { QCoapOption::LocationPath, QByteArray("path1") } };
     QVector<QCoapOption> multiple_options = {
-        { QCoapOption::LocationPath, "path2" },
-        { QCoapOption::LocationPath, "path3" }
+        { QCoapOption::LocationPath, QByteArray("path2") },
+        { QCoapOption::LocationPath, QByteArray("path3") }
     };
 
     QTest::newRow("remove_single_option") << single_option << single_option.back().name();
diff --git a/tests/auto/qcoapoption/tst_qcoapoption.cpp b/tests/auto/qcoapoption/tst_qcoapoption.cpp
index 2541fad..fec22b1 100644
--- a/tests/auto/qcoapoption/tst_qcoapoption.cpp
+++ b/tests/auto/qcoapoption/tst_qcoapoption.cpp
@@ -38,8 +38,7 @@ class tst_QCoapOption : public QObject
 
 private Q_SLOTS:
     void constructWithQByteArray();
-    void constructWithQStringView();
-    void constructWithCString();
+    void constructWithQString();
     void constructWithInteger();
     void constructWithUtf8Characters();
 };
@@ -49,23 +48,15 @@ void tst_QCoapOption::constructWithQByteArray()
     QByteArray ba = "some data";
     QCoapOption option(QCoapOption::LocationPath, ba);
 
-    QCOMPARE(option.value(), ba);
+    QCOMPARE(option.opaqueValue(), ba);
 }
 
-void tst_QCoapOption::constructWithQStringView()
+void tst_QCoapOption::constructWithQString()
 {
     QString str = "some data";
     QCoapOption option(QCoapOption::LocationPath, str);
 
-    QCOMPARE(option.value(), str.toUtf8());
-}
-
-void tst_QCoapOption::constructWithCString()
-{
-    const char *str = "some data";
-    QCoapOption option(QCoapOption::LocationPath, str);
-
-    QCOMPARE(option.value(), QByteArray(str));
+    QCOMPARE(option.opaqueValue(), str.toUtf8());
 }
 
 void tst_QCoapOption::constructWithInteger()
@@ -73,7 +64,7 @@ void tst_QCoapOption::constructWithInteger()
     quint32 value = 64000;
     QCoapOption option(QCoapOption::Size1, value);
 
-    QCOMPARE(option.valueToInt(), value);
+    QCOMPARE(option.uintValue(), value);
 }
 
 void tst_QCoapOption::constructWithUtf8Characters()
@@ -81,7 +72,7 @@ void tst_QCoapOption::constructWithUtf8Characters()
     QByteArray ba = "\xc3\xa9~\xce\xbb\xe2\x82\xb2";
     QCoapOption option(QCoapOption::LocationPath, ba);
 
-    QCOMPARE(option.value(), ba);
+    QCOMPARE(option.opaqueValue(), ba);
 }
 
 QTEST_APPLESS_MAIN(tst_QCoapOption)