[Development] [RFO QTBUG-84739] QJsonValue::fromVariant containing QByteArray

[Thiago Macieira]

Re: https://bugreports.qt.io/browse/QTBUG-84739
Summary: Qt 5.15 has an unintentional change in behaviour that has broken 
existing applications. We need to decide whether to:
 a) leave as-is
 b) revert permanently
 c) revert for 5.15.x but keep the new behaviour in 6.x

(a) implies one change in behaviour (5.14.2 to 5.15.0)
(b) implies two changes in behaviour (to and from 5.15.0)
(c) implies three changes in behaviour: 5.14.2→5.15.0, 5.15.0→5.15.1, 
5.15.x→6.0

Testcase:
    const char binarydata[] = "\0\1\2\3\xff";
    QVariantMap vmap = {
        {"ascii", QByteArray("ASCII text.\n")},
        {"latin1", QByteArray("R\xe9sum\xe9")},
        {"utf8", QByteArray("Résumé")},
        {"binary", QByteArray(binarydata, sizeof(binarydata) - 1)}
    };
    QJsonObject jobj = QJsonObject::fromVariantMap(vmap);
    printf("%s", qPrintable(QJsonDocument(jobj).toJson()));

Because of the switch in QJsonValue/Object/Array to use the CBOR classes as a 
backend, we had an uninteded change in behaviour in how the JSON 
fromVariantXxx functions behave for some variant types that are not part of 
the JSON specification. In the bug report, this happened specifically for 
QByteArray.

Qt 5.14 output:
{
    "ascii": "ASCII text.\n",
    "binary": null,
    "latin1": "R�sum�",
    "utf8": "Résumé"
}

Qt 5.15 output:
{
    "ascii": "QVNDSUkgdGV4dC4K",
    "binary": "AAECA_8",
    "latin1": "UulzdW3p",
    "utf8": "UsOpc3Vtw6k"
}

The Qt 5.15 output is the Base64url encoding of the original byte array data, 
following the CBOR recommendations on how to convert CBOR to JSON (RFC 7049 
section 4.1[1]). This was unintentional and happened only due to code reuse. 
The QtJson unit tests did not test this and do not test how converting from 
any QVariant types except the base ones that apply to JSON, plus QUrl and 
QUuid. But it has broken user code that relied on this feature.

It wasn't entirely undocumented. QJsonValue::fromVariant said:
    For all other QVariant types a conversion to a QString will be attempted.
    If the returned string
    is empty, a Null QJsonValue will be stored, otherwise a String value using
    the returned QString.
[this text is changed in https://codereview.qt-project.org/305996]

Since it depends on QVariant::toString(), it's implicit that the behaviour can 
change from version to version. For Qt 6.0, we're already discussing whether 
conversion between types in QVariant makes sense and toString() is one of 
those cases.

As can be seen in the testcase, the old behaviour is silently lossy, which 
means it's dangerous. The new behaviour is surprising, but it's not lossy, as 
the original byte data is retrievable:

	$ base64 -d <<<QVNDSUkgdGV4dC4K 
	ASCII text.
	$ base64 -d <<<UulzdW3p | od -tc
	0000000   R 351   s   u   m 351
	$ base64 -d <<<UsOpc3Vtw6k=
	Résumé                                                                                                                                          

(the type change is inevitable and is accepted by the user)

Request For Opinion: what do we do now? See options at the beginning of the 
email.

Note also this behaviour is not completely correct, because fromVariant 
converts *empty* strings to null, but it should only convert null QStrings to 
null, leaving empty strings alone (that's the QCborValue::fromVariant 
behaviour). I also don't think the implementation is self-consistent, because 
the conversion code appears more than once.

[1] https://tools.ietf.org/html/rfc7049#section-4.1
-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel System Software Products