Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix json hanlding of bools and complete json(b) PG support #30137

Merged
merged 8 commits into from Jun 10, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions python/core/auto_generated/qgsjsonutils.sip.in
Expand Up @@ -337,6 +337,7 @@ Parse a simple array (depth=1)




};

/************************************************************************
Expand Down
105 changes: 96 additions & 9 deletions src/core/qgsjsonutils.cpp
Expand Up @@ -345,15 +345,15 @@ QVariantList QgsJsonUtils::parseArray( const QString &json, QVariant::Type type
{
v = QString::fromStdString( item.get<std::string>() );
}
else if ( item.is_boolean() )
{
v = item.get<bool>();
}
else if ( item.is_null() )
{
// Fallback to int
v = QVariant( type == QVariant::Type::Invalid ? QVariant::Type::Int : type );
}
else
{
// do nothing and discard the item
}

// If a destination type was specified (it's not invalid), try to convert
if ( type != QVariant::Invalid )
Expand Down Expand Up @@ -384,6 +384,11 @@ QVariantList QgsJsonUtils::parseArray( const QString &json, QVariant::Type type

json QgsJsonUtils::jsonFromVariant( const QVariant &val )
{
if ( val.isNull() || ! val.isValid() )
{
return nullptr;
}
json j;
if ( val.type() == QVariant::Type::Map )
{
const auto vMap { val.toMap() };
Expand All @@ -392,7 +397,7 @@ json QgsJsonUtils::jsonFromVariant( const QVariant &val )
{
jMap[ it.key().toStdString() ] = jsonFromVariant( it.value() );
}
return jMap;
j = jMap;
}
else if ( val.type() == QVariant::Type::List )
{
Expand All @@ -402,7 +407,7 @@ json QgsJsonUtils::jsonFromVariant( const QVariant &val )
{
jList.push_back( jsonFromVariant( v ) );
}
return jList;
j = jList;
}
else
{
Expand All @@ -412,14 +417,96 @@ json QgsJsonUtils::jsonFromVariant( const QVariant &val )
case QMetaType::UInt:
case QMetaType::LongLong:
case QMetaType::ULongLong:
return val.toLongLong();
j = val.toLongLong();
break;
case QMetaType::Double:
case QMetaType::Float:
return val.toDouble();
j = val.toDouble();
break;
case QMetaType::Bool:
j = val.toBool();
break;
default:
return val.toString().toStdString();
j = val.toString().toStdString();
break;
}
}
return j;
}

QVariant QgsJsonUtils::parseJson( const std::string &jsonString )
{
std::function<QVariant( json )> _parser { [ & ]( json jObj ) -> QVariant {
QVariant result;
QString errorMessage;
if ( jObj.is_array() )
{
QVariantList results;
for ( const auto &item : jObj )
{
results.push_back( _parser( item ) );
}
result = results;
}
else if ( jObj.is_object() )
{
QVariantMap results;
for ( const auto &item : jObj.items() )
{
const auto key { QString::fromStdString( item.key() ) };
const auto value { _parser( item.value() ) };
results[ key ] = value;
}
result = results;
}
else
{
if ( jObj.is_number_integer() )
{
result = jObj.get<int>();
}
else if ( jObj.is_number_unsigned() )
{
result = jObj.get<unsigned>();
}
else if ( jObj.is_boolean() )
{
result = jObj.get<bool>();
}
else if ( jObj.is_number_float() )
{
// Note: it's a double and not a float on purpose
result = jObj.get<double>();
}
else if ( jObj.is_string() )
{
result = QString::fromStdString( jObj.get<std::string>() );
}
else if ( jObj.is_null() )
{
// Do nothing (leave invalid)
}
}
return result;
}
};

try
{
const json j = json::parse( jsonString );
return _parser( j );
}
catch ( json::parse_error &ex )
{
QgsLogger::warning( QStringLiteral( "Cannot parse json (%1): %2" ).arg( QString::fromStdString( ex.what() ),
QString::fromStdString( jsonString ) ) );
}
return QVariant();
}

QVariant QgsJsonUtils::parseJson( const QString &jsonString )
{
return parseJson( jsonString.toStdString() );
}

json QgsJsonUtils::exportAttributesToJsonObject( const QgsFeature &feature, QgsVectorLayer *layer, const QVector<QVariant> &attributeWidgetCaches )
Expand Down
13 changes: 13 additions & 0 deletions src/core/qgsjsonutils.h
Expand Up @@ -345,6 +345,19 @@ class CORE_EXPORT QgsJsonUtils
*/
static json jsonFromVariant( const QVariant &v ) SIP_SKIP;

/**
* Converts JSON QString \a jsonString to a QVariant, in case of parsing error an invalid QVariant is returned.
elpaso marked this conversation as resolved.
Show resolved Hide resolved
* \note Not available in Python bindings
* \since QGIS 3.8
*/
static QVariant parseJson( const std::string &jsonString ) SIP_SKIP;

/**
* Converts JSON std::string \a jsonString to a QVariant, in case of parsing error an invalid QVariant is returned.
elpaso marked this conversation as resolved.
Show resolved Hide resolved
* \note Not available in Python bindings
* \since QGIS 3.8
*/
static QVariant parseJson( const QString &jsonString ) SIP_SKIP;

};

Expand Down
14 changes: 13 additions & 1 deletion src/providers/postgres/qgspostgresconn.cpp
Expand Up @@ -27,12 +27,15 @@
#include "qgsvectordataprovider.h"
#include "qgswkbtypes.h"
#include "qgssettings.h"
#include "qgsjsonutils.h"

#include <QApplication>
#include <QThread>

#include <climits>

#include <nlohmann/json.hpp>

// for htonl
#ifdef Q_OS_WIN
#include <winsock.h>
Expand Down Expand Up @@ -992,7 +995,6 @@ static QString doubleQuotedMapValue( const QString &v )

static QString quotedMap( const QVariantMap &map )
{
//to store properly it should be decided if it's a hstore or a json/jsonb field here...
QString ret;
for ( QVariantMap::const_iterator i = map.constBegin(); i != map.constEnd(); ++i )
{
Expand Down Expand Up @@ -1057,6 +1059,16 @@ QString QgsPostgresConn::quotedValue( const QVariant &value )
}
}

QString QgsPostgresConn::quotedJsonValue( const QVariant &value )
{
if ( value.isNull() || !value.isValid() )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( value.isNull() || !value.isValid() )
if ( value.isNull() || !value.isValid() || value.type() == QVariant::Bool )
return QgsPostgresConn::quotedValue( value );

(for consistency)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few more tests, null, true and false should be handled correctly now.

return QStringLiteral( "null" );
if ( value.type() == QVariant::Bool )
return value.toBool() ? QStringLiteral( "true" ) : QStringLiteral( "false" );
const auto j { QgsJsonUtils::jsonFromVariant( value ) };
return QStringLiteral( "'%1'" ).arg( QString::fromStdString( j.dump() ) );
elpaso marked this conversation as resolved.
Show resolved Hide resolved
}

PGresult *QgsPostgresConn::PQexec( const QString &query, bool logError, bool retry ) const
{
QMutexLocker locker( &mLock );
Expand Down
5 changes: 5 additions & 0 deletions src/providers/postgres/qgspostgresconn.h
Expand Up @@ -283,6 +283,11 @@ class QgsPostgresConn : public QObject
*/
static QString quotedValue( const QVariant &value );

/**
* Quote a json(b) value for placement in a SQL string.
*/
static QString quotedJsonValue( const QVariant &value );

/**
* Gets the list of supported layers
* \param layers list to store layers in
Expand Down
26 changes: 20 additions & 6 deletions src/providers/postgres/qgspostgresprovider.cpp
Expand Up @@ -41,6 +41,7 @@
#include "qgslogger.h"
#include "qgsfeedback.h"
#include "qgssettings.h"
#include "qgsjsonutils.h"

#ifdef HAVE_GUI
#include "qgspgsourceselect.h"
Expand Down Expand Up @@ -2185,10 +2186,17 @@ bool QgsPostgresProvider::addFeatures( QgsFeatureList &flist, Flags flags )
.arg( delim,
quotedValue( v.toString() ) );
}
else if ( fieldTypeName == QLatin1String( "jsonb" ) )
{
values += delim + quotedJsonValue( v ) + QLatin1String( "::jsonb" );
}
else if ( fieldTypeName == QLatin1String( "json" ) )
{
values += delim + quotedJsonValue( v ) + QLatin1String( "::json" );
}
//TODO: convert arrays and hstore to native types
else
{
//this should be for json/jsonb in future
values += delim + quotedValue( v );
}
}
Expand Down Expand Up @@ -2734,6 +2742,16 @@ bool QgsPostgresProvider::changeAttributeValues( const QgsChangedAttributesMap &
sql += QStringLiteral( "st_geographyfromewkt(%1)" )
.arg( quotedValue( siter->toString() ) );
}
else if ( fld.typeName() == QLatin1String( "jsonb" ) )
{
sql += QStringLiteral( "%1::jsonb" )
.arg( quotedJsonValue( siter.value() ) );
}
else if ( fld.typeName() == QLatin1String( "json" ) )
{
sql += QStringLiteral( "%1::json" )
.arg( quotedJsonValue( siter.value() ) );
}
else
{
sql += quotedValue( *siter );
Expand Down Expand Up @@ -4316,11 +4334,7 @@ QVariant QgsPostgresProvider::parseHstore( const QString &txt )

QVariant QgsPostgresProvider::parseJson( const QString &txt )
{
QVariant result;
QJsonDocument jsonResponse = QJsonDocument::fromJson( txt.toUtf8() );
//it's null if no json format
result = jsonResponse.toVariant();
return result;
return QgsJsonUtils::parseJson( txt );
}

QVariant QgsPostgresProvider::parseOtherArray( const QString &txt, QVariant::Type subType, const QString &typeName )
Expand Down
1 change: 1 addition & 0 deletions src/providers/postgres/qgspostgresprovider.h
Expand Up @@ -445,6 +445,7 @@ class QgsPostgresProvider : public QgsVectorDataProvider

static QString quotedIdentifier( const QString &ident ) { return QgsPostgresConn::quotedIdentifier( ident ); }
static QString quotedValue( const QVariant &value ) { return QgsPostgresConn::quotedValue( value ); }
static QString quotedJsonValue( const QVariant &value ) { return QgsPostgresConn::quotedJsonValue( value ); }

friend class QgsPostgresFeatureSource;

Expand Down
29 changes: 29 additions & 0 deletions tests/src/core/testqgsjsonutils.cpp
Expand Up @@ -38,6 +38,7 @@ class TestQgsJsonUtils : public QObject
private slots:
void testStringList();
void testJsonArray();
void testParseJson();
void testIntList();
void testDoubleList();
void testExportAttributesJson_data();
Expand Down Expand Up @@ -91,6 +92,8 @@ void TestQgsJsonUtils::testJsonArray()
// Empty
QCOMPARE( QgsJsonUtils::parseArray( R"([])", QVariant::Int ), QVariantList() );
QCOMPARE( QgsJsonUtils::parseArray( "", QVariant::Int ), QVariantList() );
// Booleans
QCOMPARE( QgsJsonUtils::parseArray( "[true,false]", QVariant::Bool ), QVariantList() << true << false );
// Nulls
for ( const QVariant &value : QgsJsonUtils::parseArray( R"([null, null])" ) )
{
Expand All @@ -106,6 +109,32 @@ void TestQgsJsonUtils::testJsonArray()
}
}

void TestQgsJsonUtils::testParseJson()
{
QStringList tests {{
"null",
"false",
"true",
"123",
"123.45",
R"j("a string")j",
"[1,2,3.4,null]",
R"j({"_bool":true,"_double":1234.45,"_int":123,"_list":[1,2,3.4,null],"_null":null,"_object":{"int":123}})j",
}};

for ( const auto &testJson : tests )
{
const auto parsed { QgsJsonUtils::parseJson( testJson ) };
QCOMPARE( QString::fromStdString( QgsJsonUtils::jsonFromVariant( parsed ).dump() ), testJson );
}

// Test empty string: null
QCOMPARE( QString::fromStdString( QgsJsonUtils::jsonFromVariant( QgsJsonUtils::parseJson( QStringLiteral( "" ) ) ).dump() ), QString( "null" ) );
// invalid json -> null
QCOMPARE( QString::fromStdString( QgsJsonUtils::jsonFromVariant( QgsJsonUtils::parseJson( QStringLiteral( "invalid json" ) ) ).dump() ), QString( "null" ) );

}

void TestQgsJsonUtils::testIntList()
{
QVariantList list;
Expand Down