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.macie...@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@qt-project.org > https://lists.qt-project.org/listinfo/development _______________________________________________ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
