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

[Tor Arne Vestbø]

Hi,

I can sympathize with the reporter that this is a quite serious regression from the user’s point of view, but I also see your point about the new behavior not being lossy.

Would it be an alternative to add an argument to QJsonValue::fromVariant e.g. that defaults to the old, unsurprising behavior, but allows opting in to the safer approach?

I assume people who put “true” binary data into their QByteArrays and convert that to JSON would have pre-base64-encoded it manually already, to avoid the data loss, so this is a surprising change for those that just use it as a poor man’s string. 

That the documentation says "a conversion to a QString will be attempted.” reads to me as being a conversion that is as close to the original as possible, and results in a string. So while the new behavior is technically a string, I can see how it’s surprising that it’s not exactly close to the original ascii data e.g. 😊

My 2øre

Tor Arne 

> On 1 Jul 2020, at 23:21, Thiago Macieira <thiago.macieira at intel.com> wrote:
> 
> 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
> 
> 
> 
> _______________________________________________
> Development mailing list
> Development at qt-project.org
> https://lists.qt-project.org/listinfo/development