Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix spatialite handling of JSON arrays
Fixes #21986

plus:

- fix multiple string keys with commas in value relation widget
- more robust JSON and array (un)marshalling
- uniform array representation in value relation widgets
- lot of test coverage
- automatic QVariant type conversions in JSON utils
  • Loading branch information
elpaso committed May 27, 2019
1 parent d7fa028 commit 564fd50
Show file tree
Hide file tree
Showing 15 changed files with 721 additions and 181 deletions.
Expand Up @@ -59,7 +59,7 @@ Constructor for QgsValueRelationFieldFormatter.

static QStringList valueToStringList( const QVariant &value );
%Docstring
Utility to convert an array or a string representation of an array ``value`` to a string list
Utility to convert an array or a string representation of an (C style: {1,2...}) array in the form ``value`` to a string list

.. versionadded:: 3.2
%End
Expand Down
8 changes: 5 additions & 3 deletions python/core/auto_generated/qgsjsonutils.sip.in
Expand Up @@ -322,12 +322,14 @@ Exports all attributes from a QgsFeature as a JSON map type.
%End


static QVariantList parseArray( const QString &json, QVariant::Type type );
static QVariantList parseArray( const QString &json, QVariant::Type type = QVariant::Invalid );
%Docstring
Parse a simple array (depth=1).
Parse a simple array (depth=1)

:param json: the JSON to parse
:param type: the type of the elements
:param type: optional variant type of the elements, if specified (and not Invalid),
the array items will be converted to the type, and discarded if
the conversion is not possible.

.. versionadded:: 3.0
%End
Expand Down
44 changes: 43 additions & 1 deletion src/core/fieldformatter/qgsvaluerelationfieldformatter.cpp
Expand Up @@ -22,6 +22,10 @@
#include "qgsapplication.h"
#include "qgsexpressioncontextutils.h"


#include <nlohmann/json.hpp>
using json = nlohmann::json;

#include <QSettings>

bool orderByKeyLessThan( const QgsValueRelationFieldFormatter::ValueRelationItem &p1, const QgsValueRelationFieldFormatter::ValueRelationItem &p2 )
Expand Down Expand Up @@ -164,11 +168,49 @@ QgsValueRelationFieldFormatter::ValueRelationCache QgsValueRelationFieldFormatte

QStringList QgsValueRelationFieldFormatter::valueToStringList( const QVariant &value )
{
// Note: the recommended way to pass a value is QVariant::StringList but
// for back compatibility a string representation either in the form
// of a JSON array or in the form of {"quoted_str", 1, ... } is
// also accepted
QStringList checkList;
if ( value.type() == QVariant::StringList )
{
checkList = value.toStringList();
}
else if ( value.type() == QVariant::String )
checkList = value.toString().remove( QChar( '{' ) ).remove( QChar( '}' ) ).split( ',' );
{
// This must be an array representation
auto newVal { value };
if ( newVal.toString().trimmed().startsWith( '{' ) )
{
newVal = QVariant( newVal.toString().trimmed().mid( 1 ).chopped( 1 ).prepend( '[' ).append( ']' ) );
}
if ( newVal.toString().trimmed().startsWith( '[' ) )
{
try
{
for ( auto &element : json::parse( newVal.toString().toStdString() ) )
{
if ( element.is_number_integer() )
{
checkList << QString::number( element.get<int>() );
}
else if ( element.is_number_unsigned() )
{
checkList << QString::number( element.get<unsigned>() );
}
else if ( element.is_string() )
{
checkList << QString::fromStdString( element.get<std::string>() );
}
}
}
catch ( json::parse_error ex )
{
qDebug() << QString::fromStdString( ex.what() );
}
}
}
else if ( value.type() == QVariant::List )
{
QVariantList valuesList( value.toList( ) );
Expand Down
2 changes: 1 addition & 1 deletion src/core/fieldformatter/qgsvaluerelationfieldformatter.h
Expand Up @@ -66,7 +66,7 @@ class CORE_EXPORT QgsValueRelationFieldFormatter : public QgsFieldFormatter
QVariant createCache( QgsVectorLayer *layer, int fieldIndex, const QVariantMap &config ) const override;

/**
* Utility to convert an array or a string representation of an array \a value to a string list
* Utility to convert an array or a string representation of an (C style: {1,2...}) array in the form \a value to a string list
* \since QGIS 3.2
*/
static QStringList valueToStringList( const QVariant &value );
Expand Down
75 changes: 58 additions & 17 deletions src/core/qgsjsonutils.cpp
Expand Up @@ -315,28 +315,69 @@ QString QgsJsonUtils::exportAttributes( const QgsFeature &feature, QgsVectorLaye

QVariantList QgsJsonUtils::parseArray( const QString &json, QVariant::Type type )
{
QJsonParseError error;
const QJsonDocument jsonDoc = QJsonDocument::fromJson( json.toUtf8(), &error );
QString errorMessage;
QVariantList result;
if ( error.error != QJsonParseError::NoError )
try
{
QgsLogger::warning( QStringLiteral( "Cannot parse json (%1): %2" ).arg( error.errorString(), json ) );
return result;
}
if ( !jsonDoc.isArray() )
{
QgsLogger::warning( QStringLiteral( "Cannot parse json (%1) as array: %2" ).arg( error.errorString(), json ) );
return result;
const auto jObj { json::parse( json.toStdString() ) };
if ( ! jObj.is_array() )
{
throw json::parse_error::create( 0, 0, QStringLiteral( "JSON value must be an array" ).toStdString() );
}
for ( const auto &item : jObj )
{
// Create a QVariant from the array item
QVariant v;
if ( item.is_number_integer() )
{
v = item.get<int>();
}
else if ( item.is_number_unsigned() )
{
v = item.get<unsigned>();
}
else if ( item.is_number_float() )
{
// Note: it's a double and not a float on purpose
v = item.get<double>();
}
else if ( item.is_string() )
{
v = QString::fromStdString( item.get<std::string>() );
}
else if ( item.is_null() )
{
// do nothing: the default is null
}
else
{
// do nothing and discard the item
}

// If a destination type was specified (it's not invalid), try to convert
if ( type != QVariant::Invalid )
{
if ( ! v.convert( static_cast<int>( type ) ) )
{
QgsLogger::warning( QStringLiteral( "Cannot convert json array element to specified type, ignoring: %1" ).arg( v.toString() ) );
}
else
{
result.push_back( v );
}
}
else
{
result.push_back( v );
}
}
}
const auto constArray = jsonDoc.array();
for ( const QJsonValue &cur : constArray )
catch ( json::parse_error ex )
{
QVariant curVariant = cur.toVariant();
if ( curVariant.convert( type ) )
result.append( curVariant );
else
QgsLogger::warning( QStringLiteral( "Cannot convert json array element: %1" ).arg( cur.toString() ) );
errorMessage = ex.what();
QgsLogger::warning( QStringLiteral( "Cannot parse json (%1): %2" ).arg( ex.what(), json ) );
}

return result;
}

Expand Down
10 changes: 6 additions & 4 deletions src/core/qgsjsonutils.h
Expand Up @@ -328,18 +328,20 @@ class CORE_EXPORT QgsJsonUtils
const QVector<QVariant> &attributeWidgetCaches = QVector<QVariant>() ) SIP_SKIP;

/**
* Parse a simple array (depth=1).
* Parse a simple array (depth=1)
* \param json the JSON to parse
* \param type the type of the elements
* \param type optional variant type of the elements, if specified (and not Invalid),
* the array items will be converted to the type, and discarded if
* the conversion is not possible.
* \since QGIS 3.0
*/
static QVariantList parseArray( const QString &json, QVariant::Type type );
static QVariantList parseArray( const QString &json, QVariant::Type type = QVariant::Invalid );


/**
* Converts a QVariant \a v to a json object
* \note Not available in Python bindings
* \since QGIS 3.10
* \since QGIS 3.8
*/
static json jsonFromVariant( const QVariant &v ) SIP_SKIP;

Expand Down
47 changes: 36 additions & 11 deletions src/gui/editorwidgets/qgsvaluerelationwidgetwrapper.cpp
Expand Up @@ -25,6 +25,7 @@
#include "qgsvaluerelationfieldformatter.h"
#include "qgsattributeform.h"
#include "qgsattributes.h"
#include "qgsjsonutils.h"

#include <QHeaderView>
#include <QComboBox>
Expand All @@ -33,6 +34,10 @@
#include <QStringListModel>
#include <QCompleter>

#include <nlohmann/json.hpp>
using json = nlohmann::json;


QgsValueRelationWidgetWrapper::QgsValueRelationWidgetWrapper( QgsVectorLayer *layer, int fieldIdx, QWidget *editor, QWidget *parent )
: QgsEditorWidgetWrapper( layer, fieldIdx, editor, parent )
{
Expand Down Expand Up @@ -70,21 +75,26 @@ QVariant QgsValueRelationWidgetWrapper::value() const
}
}

if ( layer()->fields().at( fieldIdx() ).type() == QVariant::Map )
QVariantList vl;
//store as QVariantList because it's json
for ( const QString &s : qgis::as_const( selection ) )
{
QVariantList vl;
//store as QVariantList because it's json
for ( const QString &s : qgis::as_const( selection ) )
// Convert to proper type
const QVariant::Type type { fkType() };
switch ( type )
{
vl << s;
case QVariant::Type::Int:
vl.push_back( s.toInt() );
break;
case QVariant::Type::LongLong:
vl.push_back( s.toLongLong() );
break;
default:
vl.push_back( s );
break;
}
v = vl;
}
else

This comment has been minimized.

Copy link
@signedav

signedav Jul 4, 2019

Contributor

Hi @elpaso
Sorry for commenting such an old PR. But what is the reason to remove this else-clause? Do you remember?
I guess it needs to convert somewhere the list into a string in hstore-format to be able to use it (if JSON not used). But honestly I'm confused...

This comment has been minimized.

Copy link
@elpaso

elpaso Jul 4, 2019

Author Contributor

I don't remember much, but I believe that a widget should not be responsible for transforming the values to a provider-compatible format (hstore). That transformation should be done at the provider level, hstore is AFAIK specific to postgres, other providers may have other storage formats (json?).

This comment has been minimized.

Copy link
@signedav

signedav Jul 4, 2019

Contributor

Thanks for your reply.

I agree, but in case that JSON is not used but a simple string instead (e.g. because the user has a GPKG that has been created before GDAL 2.4) means the type is not a QVariant::Map, the values has been stored as a string in this format ´{1,2,3}´(the naming "hstore-string" is confusing just because this format is converted to hstore in pg-provider, since it's used for the value relation format). If it does not store the string in this format, it's not able to handle the stored QVariantList instead.

I guess, without knowing all the details, that it would make sense to use a json-list format anywhere, but to have it fixed meanwhile, bringing back this else-clause would make it. It would have no influence to a workflow using JSON anyway...

This comment has been minimized.

Copy link
@signedav

signedav Jul 4, 2019

Contributor

Btw. it works for Postgres - there it's converted from the QVariantList - probably we have to solve it in the OGR provider. But the whole decoding part is done in the widget as well at the moment.

This comment has been minimized.

Copy link
@elpaso

elpaso Jul 4, 2019

Author Contributor

I would say that as long as you don't break any existing test and you write new test cases for this, go ahead.

{
//store as hstore string
v = selection.join( ',' ).prepend( '{' ).append( '}' );
}
v = vl;
}

if ( mLineEdit )
Expand Down Expand Up @@ -289,6 +299,21 @@ int QgsValueRelationWidgetWrapper::columnCount() const
return std::max( 1, config( QStringLiteral( "NofColumns" ) ).toInt() );
}

QVariant::Type QgsValueRelationWidgetWrapper::fkType() const
{
QgsVectorLayer *layer = QgsProject::instance()->mapLayer<QgsVectorLayer *>( config().value( QStringLiteral( "Layer" ) ).toString() );
if ( layer )
{
QgsFields fields = layer->fields();
int idx { fields.lookupField( config().value( QStringLiteral( "Key" ) ).toString() ) };
if ( idx >= 0 )
{
return fields.at( idx ).type();
}
}
return QVariant::Type::Invalid;
}

void QgsValueRelationWidgetWrapper::populate( )
{
// Initialize, note that signals are blocked, to avoid double signals on new features
Expand Down
3 changes: 3 additions & 0 deletions src/gui/editorwidgets/qgsvaluerelationwidgetwrapper.h
Expand Up @@ -115,6 +115,9 @@ class GUI_EXPORT QgsValueRelationWidgetWrapper : public QgsEditorWidgetWrapper
*/
int columnCount() const;

//! Returns the variant type of the fk
QVariant::Type fkType() const;

//! Sets the values for the widgets, re-creates the cache when required
void populate( );

Expand Down
5 changes: 4 additions & 1 deletion src/providers/spatialite/qgsspatialitefeatureiterator.cpp
Expand Up @@ -581,7 +581,10 @@ QVariant QgsSpatiaLiteFeatureIterator::getFeatureAttribute( sqlite3_stmt *stmt,
{
// assume arrays are stored as JSON
QVariant result = QVariant( QgsJsonUtils::parseArray( txt, subType ) );
result.convert( type );
if ( ! result.convert( static_cast<int>( type ) ) )
{
QgsDebugMsgLevel( QStringLiteral( "Could not convert JSON value to requested QVariant type" ).arg( txt ), 3 );
}
return result;
}
return txt;
Expand Down
31 changes: 29 additions & 2 deletions src/providers/spatialite/qgsspatialiteprovider.cpp
Expand Up @@ -44,6 +44,9 @@ email : a.furieri@lqt.it
#include <QDir>
#include <QRegularExpression>

#include <nlohmann/json.hpp>
using json = nlohmann::json;


const QString SPATIALITE_KEY = QStringLiteral( "spatialite" );
const QString SPATIALITE_DESCRIPTION = QStringLiteral( "SpatiaLite data provider" );
Expand Down Expand Up @@ -663,6 +666,10 @@ static TypeSubType getVariantType( const QString &type )
type.length() - SPATIALITE_ARRAY_PREFIX.length() - SPATIALITE_ARRAY_SUFFIX.length() ) );
return TypeSubType( subType.first == QVariant::String ? QVariant::StringList : QVariant::List, subType.first );
}
else if ( type == QLatin1String( "jsonarray" ) )
{
return TypeSubType( QVariant::List, QVariant::Invalid );
}
else
// for sure any SQLite value can be represented as SQLITE_TEXT
return TypeSubType( QVariant::String, QVariant::Invalid );
Expand Down Expand Up @@ -4385,6 +4392,7 @@ bool QgsSpatiaLiteProvider::changeAttributeValues( const QgsChangedAttributesMap
first = false;

QVariant::Type type = fld.type();
const auto typeName { fld.typeName() };

if ( val.isNull() || !val.isValid() )
{
Expand All @@ -4398,8 +4406,27 @@ bool QgsSpatiaLiteProvider::changeAttributeValues( const QgsChangedAttributesMap
}
else if ( type == QVariant::StringList || type == QVariant::List )
{
// binding an array value
sql += QStringLiteral( "%1=%2" ).arg( QgsSqliteUtils::quotedIdentifier( fld.name() ), QgsSqliteUtils::quotedString( QgsJsonUtils::encodeValue( val ) ) );
// binding an array value, parse JSON
QString jRepr;
try
{
const auto jObj { QgsJsonUtils::jsonFromVariant( val ) };
if ( ! jObj.is_array() )
{
throw json::parse_error::create( 0, 0, tr( "JSON value must be an array" ).toStdString() );
}
jRepr = QString::fromStdString( jObj.dump( ) );
sql += QStringLiteral( "%1=%2" ).arg( QgsSqliteUtils::quotedIdentifier( fld.name() ), QgsSqliteUtils::quotedString( jRepr ) );
}
catch ( json::exception ex )
{
const auto errM { tr( "Field type is JSON but the value cannot be converted to JSON array: %1" ).arg( ex.what() ) };
auto msgPtr { static_cast<char *>( sqlite3_malloc( errM.length() + 1 ) ) };
strcpy( static_cast<char *>( msgPtr ), errM.toStdString().c_str() );
errMsg = msgPtr;
handleError( jRepr, errMsg, true );
return false;
}
}
else
{
Expand Down

0 comments on commit 564fd50

Please sign in to comment.