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

[Jason H]

B. 
- This will break so many things in a big way. It does not reflect the reasonable assumption that for reasonable cases you'll get a reasonable response. I do agree it is the proper loss-less way, but people shoving extended chars in to byte arrays do so intentionally, even if haphazardly. For example, when I do it, I base64 at the application level myself. The developer should be be left to implement this level of safety themselves, not the toolit. 

- Other packages (like Python) do not automatically encode to base64. Therefore this is not expected behavior
- Also, there is no way to know to transparently decode the JSON on deserialization
- Any software that interacts with Qt serialized data will have to implement Qt's rules.

Therefore B is the only option I find acceptable. If the developer requires perfect lossless storage using byte array then they have to implement themselves, via a mechanism of their choice.

 

> Sent: Wednesday, July 01, 2020 at 5:21 PM
> From: "Thiago Macieira" <thiago.macieira at intel.com>
> To: development at qt-project.org
> Subject: [Development] [RFO QTBUG-84739] QJsonValue::fromVariant containing QByteArray
>
> 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
>