diff options
author | Sona Kurazyan <sona.kurazyan@qt.io> | 2019-05-14 13:07:23 +0200 |
---|---|---|
committer | Sona Kurazyan <sona.kurazyan@qt.io> | 2019-05-14 15:15:56 +0000 |
commit | 273f3fa6cee9c215fcdea6b5ee52ad34eb146452 (patch) | |
tree | 45ca1aef7a815f682f7f4f30336369c19d727de8 | |
parent | c90feba464526c9d00ccc63c0f372963fffc8d21 (diff) |
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>
-rw-r--r-- | src/coap/qcoapinternalmessage.cpp | 2 | ||||
-rw-r--r-- | src/coap/qcoapinternalreply.cpp | 2 | ||||
-rw-r--r-- | src/coap/qcoapinternalrequest.cpp | 2 | ||||
-rw-r--r-- | src/coap/qcoapoption.cpp | 122 | ||||
-rw-r--r-- | src/coap/qcoapoption.h | 18 | ||||
-rw-r--r-- | src/coap/qcoapoption_p.h | 4 | ||||
-rw-r--r-- | tests/auto/qcoapinternalreply/tst_qcoapinternalreply.cpp | 2 | ||||
-rw-r--r-- | tests/auto/qcoapinternalrequest/tst_qcoapinternalrequest.cpp | 31 | ||||
-rw-r--r-- | tests/auto/qcoapmessage/tst_qcoapmessage.cpp | 22 | ||||
-rw-r--r-- | tests/auto/qcoapoption/tst_qcoapoption.cpp | 21 |
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) |