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

[Tor Arne Vestbø]

On 1 Jul 2020, at 23:21, Thiago Macieira <thiago.macieira at intel.com<mailto:thiago.macieira at intel.com>> wrote:


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.

Looking at this some more, the documented behaviour is:

"For all other QVariant<https://doc.qt.io/qt-5/qvariant.html> types a conversion to a QString<https://doc.qt.io/qt-5/qstring.html> will be attempted. If the returned string is empty, a Null QJsonValue<https://doc.qt.io/qt-5/qjsonvalue.html> will be stored, otherwise a String value using the returned QString<https://doc.qt.io/qt-5/qstring.html>."

auto variant = QVariant::fromValue(QByteArray("ASCII text.\n"));
auto stringForJson = variant.toString(); // "a conversion to a QString will be attempted"
printf("%s", qPrintable(stringForJson));

Which for me results in printing "ASCII text.”

The new behavior injects an extra step here:

variant = QVariant::fromValue(QByteArray("ASCII text.\n"));
auto base64Encoded = variant.toByteArray().toBase64(); // <-----
auto stringForJsonBase64 = QString::fromUtf8(base64Encoded);
printf("%s", qPrintable(stringForJsonBase64));

Which does not match the documented behavior, so I think a revert makes sense to avoid the surprising behavior change.

Especially since users who encode true binary data will do their own (base64)-encoding anyways.

We can document that this might result in data loss and recommend pre-encoding the data. At least the user is in control then.

Tor Arne


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<http://intel.com>
 Software Architect - Intel System Software Products



_______________________________________________
Development mailing list
Development at qt-project.org<mailto:Development at qt-project.org>
https://lists.qt-project.org/listinfo/development

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20200702/3334fc4e/attachment-0001.html>