Apply the PIMPL idiom to QFormDataPartBuilder

Change QFormDataBuilderPrivate to store a vector of
QFormDataPartBuilderPrivates and pimplify QFormDataPartBuilder such that
it stores a pointer to the QFormDataBuilderPrivate and an index into the
vector. This makes it robust against QFDP::m_parts reallocations and we
can make QFDBP copyable which has the benefit that if the user wants to
"go back" to an earlier part, she can do so by storing the result of the
original part() call by value.

Found in API review.

Task-number: QTBUG-125985
Change-Id: I56e9018e539457e9494751bdb62509f84a680889
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
(cherry picked from commit aca8235c753d673abe7442cdf8b628fe4e05c471)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/network/access/qformdatabuilder.cpp

@@ -30,6 +30,26 @@ QT_BEGIN_NAMESPACE
     \sa QHttpPart, QHttpMultiPart, QFormDataBuilder
 */
 
+class QFormDataPartBuilderPrivate
+{
+public:
+    explicit QFormDataPartBuilderPrivate(QAnyStringView name);
+    QHttpPart build();
+
+    QString m_name;
+    QByteArray m_mimeType;
+    QString m_originalBodyName;
+    QHttpHeaders m_httpHeaders;
+    std::variant<QIODevice*, QByteArray> m_body;
+};
+
+QFormDataPartBuilderPrivate::QFormDataPartBuilderPrivate(QAnyStringView name)
+    : m_name{name.toString()}
+{
+
+}
+
+
 static void escapeNameAndAppend(QByteArray &dst, QByteArrayView src)
 {
     for (auto c : src) {
@@ -56,17 +76,6 @@ static void escapeNameAndAppend(QByteArray &dst, QStringView src)
     dst += QUtf8::convertFromUnicode(src.sliced(last));
 }
 
-/*!
-    Constructs a QFormDataPartBuilder object and sets \a name as the name
-    parameter of the form-data.
-*/
-QFormDataPartBuilder::QFormDataPartBuilder(QAnyStringView name, PrivateConstructor /*unused*/)
-    : m_name{name.toString()}
-{
-    static_assert(std::is_nothrow_move_constructible_v<decltype(m_body)>);
-    static_assert(std::is_nothrow_move_assignable_v<decltype(m_body)>);
-}
-
 /*!
     \fn QFormDataPartBuilder::QFormDataPartBuilder(QFormDataPartBuilder &&other) noexcept
 
@@ -81,11 +90,56 @@ QFormDataPartBuilder::QFormDataPartBuilder(QAnyStringView name, PrivateConstruct
 */
 
 /*!
-    Destroys the QFormDataPartBuilder object.
+    \fn QFormDataPartBuilder::QFormDataPartBuilder(const QFormDataPartBuilder &other)
+
+    Constructs a copy of \a other. The object is valid for as long as the associated
+    QFormDataBuilder has not been destroyed.
+
+    The data of the copy is shared (shallow copy): modifying one part will also change
+    the other.
+
+    \code
+        QFormDataPartBuilder foo()
+        {
+            QFormDataBuilder builder;
+            auto qfdpb1 = builder.part("First"_L1);
+            auto qfdpb2 = qfdpb1; // this creates a shallow copy
+
+            qfdpb2.setBodyDevice(&image, "cutecat.jpg"); // qfdpb1 is also modified
+
+            return qfdbp2;  // invalid, builder is destroyed at the end of the scope
+        }
+    \endcode
 */
 
-QFormDataPartBuilder::~QFormDataPartBuilder()
+/*!
-    = default;
+    \fn QFormDataPartBuilder& QFormDataPartBuilder::operator=(const QFormDataPartBuilder &other)
+
+    Assigns \a other to QFormDataPartBuilder and returns a reference to this
+    QFormDataPartBuilder. The object is valid for as long as the associated QFormDataBuilder
+    has not been destroyed.
+
+    The data of the copy is shared (shallow copy): modifying one part will also change the other.
+
+    \code
+        QFormDataPartBuilder foo()
+        {
+            QFormDataBuilder builder;
+            auto qfdpb1 = builder.part("First"_L1);
+            auto qfdpb2 = qfdpb1; // this creates a shallow copy
+
+            qfdpb2.setBodyDevice(&image, "cutecat.jpg"); // qfdpb1 is also modified
+
+            return qfdbp2;  // invalid, builder is destroyed at the end of the scope
+        }
+    \endcode
+*/
+
+/*!
+    \fn QFormDataPartBuilder::~QFormDataPartBuilder()
+
+    Destroys the QFormDataPartBuilder object.
+*/
 
 static void convertInto_impl(QByteArray &dst, QUtf8StringView in)
 {
@@ -110,13 +164,15 @@ static void convertInto(QByteArray &dst, QAnyStringView in)
     in.visit([&dst](auto in) { convertInto_impl(dst, in); });
 }
 
-QFormDataPartBuilder &QFormDataPartBuilder::setBodyHelper(const QByteArray &data,
+QFormDataPartBuilder QFormDataPartBuilder::setBodyHelper(const QByteArray &data,
                                                          QAnyStringView name,
                                                          QAnyStringView mimeType)
 {
-    m_originalBodyName = name.toString();
+    Q_D(QFormDataPartBuilder);
-    convertInto(m_mimeType, mimeType);
+
-    m_body = data;
+    d->m_originalBodyName = name.toString();
+    convertInto(d->m_mimeType, mimeType);
+    d->m_body = data;
     return *this;
 }
 
@@ -136,7 +192,7 @@ QFormDataPartBuilder &QFormDataPartBuilder::setBodyHelper(const QByteArray &data
     \sa setBodyDevice()
 */
 
-QFormDataPartBuilder &QFormDataPartBuilder::setBody(QByteArrayView data,
+QFormDataPartBuilder QFormDataPartBuilder::setBody(QByteArrayView data,
                                                    QAnyStringView fileName,
                                                    QAnyStringView mimeType)
 {
@@ -168,12 +224,14 @@ QFormDataPartBuilder &QFormDataPartBuilder::setBody(QByteArrayView data,
     \sa setBody(), QHttpPart::setBodyDevice()
   */
 
-QFormDataPartBuilder &QFormDataPartBuilder::setBodyDevice(QIODevice *body, QAnyStringView fileName,
+QFormDataPartBuilder QFormDataPartBuilder::setBodyDevice(QIODevice *body, QAnyStringView fileName,
                                                          QAnyStringView mimeType)
 {
-    m_originalBodyName = fileName.toString();
+    Q_D(QFormDataPartBuilder);
-    convertInto(m_mimeType, mimeType);
+
-    m_body = body;
+    d->m_originalBodyName = fileName.toString();
+    convertInto(d->m_mimeType, mimeType);
+    d->m_body = body;
     return *this;
 }
 
@@ -184,9 +242,11 @@ QFormDataPartBuilder &QFormDataPartBuilder::setBodyDevice(QIODevice *body, QAnyS
     specified in \a headers, will be overwritten by the class.
 */
 
-QFormDataPartBuilder &QFormDataPartBuilder::setHeaders(const QHttpHeaders &headers)
+QFormDataPartBuilder QFormDataPartBuilder::setHeaders(const QHttpHeaders &headers)
 {
-    m_httpHeaders = headers;
+    Q_D(QFormDataPartBuilder);
+
+    d->m_httpHeaders = headers;
     return *this;
 }
 
@@ -200,7 +260,7 @@ QFormDataPartBuilder &QFormDataPartBuilder::setHeaders(const QHttpHeaders &heade
     header.
 */
 
-QHttpPart QFormDataPartBuilder::build()
+QHttpPart QFormDataPartBuilderPrivate::build()
 {
     QHttpPart httpPart;
 
@@ -292,9 +352,20 @@ QHttpPart QFormDataPartBuilder::build()
 class QFormDataBuilderPrivate
 {
 public:
-    std::vector<QFormDataPartBuilder> parts;
+    std::vector<QFormDataPartBuilderPrivate> parts;
 };
 
+QFormDataPartBuilderPrivate* QFormDataPartBuilder::d_func()
+{
+    return const_cast<QFormDataPartBuilderPrivate*>(std::as_const(*this).d_func());
+}
+
+const QFormDataPartBuilderPrivate* QFormDataPartBuilder::d_func() const
+{
+    Q_ASSERT(m_index < d->parts.size());
+    return &d->parts[m_index];
+}
+
 /*!
     Constructs an empty QFormDataBuilder object.
 */
@@ -328,9 +399,9 @@ QFormDataBuilder::~QFormDataBuilder()
 */
 
 /*!
-    Constructs and returns a reference to a QFormDataPartBuilder object and sets
+    Returns a newly-constructed QFormDataPartBuilder object using \a name as the
-    \a name as the name parameter of the form-data. The returned reference is
+    form-data's \c name parameter. The object is valid for as long as the
-    valid until the next call to this function.
+    associated QFormDataBuilder has not been destroyed.
 
     Limiting \a name characters to US-ASCII is
     \l {https://datatracker.ietf.org/doc/html/rfc7578#section-5.1.1}{strongly recommended}
@@ -339,11 +410,12 @@ QFormDataBuilder::~QFormDataBuilder()
     \sa QFormDataPartBuilder, QHttpPart
 */
 
-QFormDataPartBuilder &QFormDataBuilder::part(QAnyStringView name)
+QFormDataPartBuilder QFormDataBuilder::part(QAnyStringView name)
 {
     Q_D(QFormDataBuilder);
 
-    return d->parts.emplace_back(name, QFormDataPartBuilder::PrivateConstructor());
+    d->parts.emplace_back(name);
+    return QFormDataPartBuilder(d, d->parts.size() - 1);
 }
 
 /*!

src/network/access/qformdatabuilder.h

@@ -26,67 +26,49 @@ class QHttpMultiPart;
 class QDebug;
 
 class QFormDataBuilderPrivate;
+class QFormDataPartBuilderPrivate;
 
 class QFormDataPartBuilder
 {
-    struct PrivateConstructor { explicit PrivateConstructor() = default; };
+    QFormDataPartBuilder(QFormDataBuilderPrivate *qfdb, qsizetype idx) : d(qfdb), m_index(idx) {}
 public:
-    Q_NETWORK_EXPORT explicit QFormDataPartBuilder(QAnyStringView name, PrivateConstructor);
-
-    QFormDataPartBuilder(QFormDataPartBuilder &&other) noexcept
-        : m_name(std::move(other.m_name)),
-          m_originalBodyName(std::move(other.m_originalBodyName)),
-          m_httpHeaders(std::move(other.m_httpHeaders)),
-          m_body(std::move(other.m_body)),
-          m_reserved(std::exchange(other.m_reserved, nullptr))
-    {
-
-    }
-
-    QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_PURE_SWAP(QFormDataPartBuilder)
     void swap(QFormDataPartBuilder &other) noexcept
     {
-        m_name.swap(other.m_name);
+        qt_ptr_swap(d, other.d);
-        m_originalBodyName.swap(other.m_originalBodyName);
+        std::swap(m_index, other.m_index);
-        m_httpHeaders.swap(other.m_httpHeaders);
-        m_body.swap(other.m_body);
-        qt_ptr_swap(m_reserved, other.m_reserved);
     }
 
-    Q_NETWORK_EXPORT ~QFormDataPartBuilder();
+    QFormDataPartBuilder() = default;
+    // Rule of zero applies
 
-    Q_WEAK_OVERLOAD QFormDataPartBuilder &setBody(const QByteArray &data,
+    Q_WEAK_OVERLOAD QFormDataPartBuilder setBody(const QByteArray &data,
                                                  QAnyStringView fileName = {},
                                                  QAnyStringView mimeType = {})
     { return setBodyHelper(data, fileName, mimeType); }
 
-    Q_NETWORK_EXPORT QFormDataPartBuilder &setBody(QByteArrayView data,
+    Q_NETWORK_EXPORT QFormDataPartBuilder setBody(QByteArrayView data,
                                                   QAnyStringView fileName = {},
                                                   QAnyStringView mimeType = {});
-    Q_NETWORK_EXPORT QFormDataPartBuilder &setBodyDevice(QIODevice *body,
+    Q_NETWORK_EXPORT QFormDataPartBuilder setBodyDevice(QIODevice *body,
                                                         QAnyStringView fileName = {},
                                                         QAnyStringView mimeType = {});
-    Q_NETWORK_EXPORT QFormDataPartBuilder &setHeaders(const QHttpHeaders &headers);
+    Q_NETWORK_EXPORT QFormDataPartBuilder setHeaders(const QHttpHeaders &headers);
 private:
-    Q_DISABLE_COPY(QFormDataPartBuilder)
+    Q_NETWORK_EXPORT QFormDataPartBuilder setBodyHelper(const QByteArray &data,
-
-    Q_NETWORK_EXPORT QFormDataPartBuilder &setBodyHelper(const QByteArray &data,
                                                         QAnyStringView fileName,
                                                         QAnyStringView mimeType);
-    QHttpPart build();
 
-    QString m_name;
+    QFormDataPartBuilderPrivate* d_func();
-    QByteArray m_mimeType;
+    const QFormDataPartBuilderPrivate* d_func() const;
-    QString m_originalBodyName;
+
-    QHttpHeaders m_httpHeaders;
+    QFormDataBuilderPrivate *d;
-    std::variant<QIODevice*, QByteArray> m_body;
+    size_t m_index;
-    void *m_reserved = nullptr;
 
     friend class QFormDataBuilder;
-    friend void swap(QFormDataPartBuilder &lhs, QFormDataPartBuilder &rhs) noexcept
-    { lhs.swap(rhs); }
 };
 
+Q_DECLARE_SHARED(QFormDataPartBuilder)
+
 class QFormDataBuilder
 {
 public:
@@ -101,7 +83,7 @@ public:
     }
 
     Q_NETWORK_EXPORT ~QFormDataBuilder();
-    Q_NETWORK_EXPORT QFormDataPartBuilder &part(QAnyStringView name);
+    Q_NETWORK_EXPORT QFormDataPartBuilder part(QAnyStringView name);
     Q_NETWORK_EXPORT std::unique_ptr<QHttpMultiPart> buildMultiPart();
 private:
     QFormDataBuilderPrivate *d_ptr;

tests/auto/network/access/qformdatabuilder/tst_qformdatabuilder.cpp

@@ -79,6 +79,7 @@ private Q_SLOTS:
     void picksUtf8NameEncodingIfAsciiDoesNotSuffice();
 
     void moveSemantics();
+    void keepResultOfCallingPartAliveAmongSubsequentCallsToPart();
 };
 
 void tst_QFormDataBuilder::checkBodyPartsAreEquivalent(QByteArrayView expected, QByteArrayView actual)
@@ -335,7 +336,7 @@ void tst_QFormDataBuilder::setHeadersDoesNotAffectHeaderFieldsManagedByBuilder()
     QVERIFY(buff.open(QIODevice::ReadOnly));
 
     const auto msg = serialized([&](auto &builder) {
-            auto &qfdpb = builder.part(name_data).setBodyDevice(&buff, body_name_data);
+            auto qfdpb = builder.part(name_data).setBodyDevice(&buff, body_name_data);
 
             if (overwrite || extra_headers) {
                 QHttpHeaders headers;
@@ -383,7 +384,7 @@ void tst_QFormDataBuilder::specifyMimeType()
     QVERIFY(buff.open(QIODevice::ReadOnly));
 
     const auto msg = serialized([&](auto &builder) {
-            auto &qfdpb = builder.part(name_data).setBodyDevice(&buff, body_name_data);
+            auto qfdpb = builder.part(name_data).setBodyDevice(&buff, body_name_data);
 
             if (!mime_type.empty())
                 qfdpb.setBodyDevice(&buff, body_name_data, mime_type);
@@ -452,7 +453,7 @@ void tst_QFormDataBuilder::moveSemantics()
         QVERIFY2(data_file.open(QIODeviceBase::ReadOnly), qPrintable(data_file.errorString()));
 
         QFormDataBuilder qfdb;
-        auto &p = qfdb.part("text"_L1);
+        auto p = qfdb.part("text"_L1);
         const QByteArray actual = serialized([&, moved = std::move(qfdb)](auto &) mutable {
                 p.setBodyDevice(&data_file, "rfc3252.txt");
                 return std::ref(moved);
@@ -477,5 +478,39 @@ void tst_QFormDataBuilder::moveSemantics()
     }
 }
 
+void tst_QFormDataBuilder::keepResultOfCallingPartAliveAmongSubsequentCallsToPart()
+{
+    QFormDataBuilder qfdb;
+    auto p1 = qfdb.part("1"_L1);
+    auto p2 = qfdb.part("2"_L1);
+    auto p3 = qfdb.part("3"_L1);
+    auto p4 = qfdb.part("4"_L1);
+    auto p5 = qfdb.part("5"_L1);
+    auto p6 = qfdb.part("6"_L1);
+    auto p7 = qfdb.part("7"_L1);
+    auto p8 = qfdb.part("8"_L1);
+    auto p9 = qfdb.part("9"_L1);
+    auto p10 = qfdb.part("10"_L1);
+    auto p11 = qfdb.part("11"_L1);
+    auto p12 = qfdb.part("12"_L1);
+
+    QByteArray dummyData = "totally_a_text_file"_ba;
+
+    p1.setBody(dummyData, "body1"_L1);
+    p2.setBody(dummyData, "body2"_L1);
+    p3.setBody(dummyData, "body3"_L1);
+    p4.setBody(dummyData, "body4"_L1);
+    p5.setBody(dummyData, "body5"_L1);
+    p6.setBody(dummyData, "body6"_L1);
+    p7.setBody(dummyData, "body7"_L1);
+    p8.setBody(dummyData, "body8"_L1);
+    p9.setBody(dummyData, "body9"_L1);
+    p10.setBody(dummyData, "body10"_L1);
+    p11.setBody(dummyData, "body11"_L1);
+    p12.setBody(dummyData, "body12"_L1);
+
+    qfdb.buildMultiPart();
+}
+
 QTEST_MAIN(tst_QFormDataBuilder)
 #include "tst_qformdatabuilder.moc"