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.

Sign up for GitHub

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

Jump to bottom

Dataprovider ogr spatialite respect provider defaults #34012

Merged
merged 8 commits into from Jan 29, 2020
Merged

Dataprovider ogr spatialite respect provider defaults #34012

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
05c6150
OGR: respect provider default values
elpaso Jan 23, 2020
13681cc
Spatialite: respect provider default values
elpaso Jan 23, 2020
f259345
Fix ogr and spatialite default values
elpaso Jan 23, 2020
ebf7d24
Provider default values: more tests and homogenize
elpaso Jan 24, 2020
f8e255e
Spelling
elpaso Jan 24, 2020
439a3aa
Code layout and revert default value for OGR
elpaso Jan 28, 2020
171c522
Fix spatialite PKs autoincrement backticks
elpaso Jan 28, 2020
be63036
Update tests for defaultValue/Clause
elpaso Jan 28, 2020
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
23 changes: 13 additions & 10 deletions src/core/providers/ogr/qgsogrprovider.cpp
View file
Open in desktop
Expand Up @@ -1209,13 +1209,18 @@ void QgsOgrProvider::loadFields()
QString defaultValue = textEncoding()->toUnicode( OGR_Fld_GetDefault( fldDef ) );
if ( !defaultValue.isEmpty() && !OGR_Fld_IsDefaultDriverSpecific( fldDef ) )
{
if ( defaultValue.startsWith( '\'' ) )
{
defaultValue = defaultValue.remove( 0, 1 );
defaultValue.chop( 1 );
defaultValue.replace( QLatin1String( "''" ), QLatin1String( "'" ) );
}
mDefaultValues.insert( createdFields, defaultValue );
}

mAttributeFields.append( newField );
createdFields++;
}

}


Expand Down Expand Up @@ -1380,7 +1385,7 @@ QVariant QgsOgrProvider::defaultValue( int fieldId ) const

QString defaultVal = mDefaultValues.value( fieldId, QString() );
if ( defaultVal.isEmpty() )
return QVariant();
return defaultVal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this change is correct -- there's lots of existing code which checks if the returned value isValid(), and now that will return true instead of false (yet the default value doesn't exist)

Copy link
Member

Choose a reason for hiding this comment

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

And wouldn't the check for mDefaultValues.hasKey be correct here?
Not that an empty string as default value is common, but it's at least in SQL terms it's valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an attempt to homogenize the behavior, I will revert it and give up with the attempt.

But if you have a look to the tests, you'll see that the actual behavior of the two methods it is totally different between providers (I tested PG ogr and spatialite, but I expect that the others will not behave better).

What they return depends on a permutation of:

  • the fact that it is a literal default or not
  • the fact that it is a particular function/magic-value or not (CURRENT_TIMESTAMP for instance)
  • which provider (and maybe which driver for OGR)
  • if EvaluateDefaultValues is honored and its value

The least we can do is to document it in the API and possibly try to fix it for QGIS 4.


QVariant resultVar = defaultVal;
if ( defaultVal == QStringLiteral( "CURRENT_TIMESTAMP" ) )
Expand All @@ -1389,13 +1394,6 @@ QVariant QgsOgrProvider::defaultValue( int fieldId ) const
resultVar = QDate::currentDate();
else if ( defaultVal == QStringLiteral( "CURRENT_TIME" ) )
resultVar = QTime::currentTime();
else if ( defaultVal.startsWith( '\'' ) )
{
defaultVal = defaultVal.remove( 0, 1 );
defaultVal.chop( 1 );
defaultVal.replace( QLatin1String( "''" ), QLatin1String( "'" ) );
resultVar = defaultVal;
}

( void )mAttributeFields.at( fieldId ).convertCompatible( resultVar );
return resultVar;
Expand Down Expand Up @@ -1584,7 +1582,12 @@ bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags )
OGRFieldType type = OGR_Fld_GetType( fldDef );

QVariant attrVal = attrs.at( qgisAttId );
if ( attrVal.isNull() || ( type != OFTString && attrVal.toString().isEmpty() ) )
// The field value is equal to the default (that might be a provider-side expression)
if ( mDefaultValues.contains( qgisAttId ) && attrVal.toString() == mDefaultValues.value( qgisAttId ) )
{
OGR_F_UnsetField( feature.get(), ogrAttId );
}
else if ( attrVal.isNull() || ( type != OFTString && attrVal.toString().isEmpty() ) )
{
// Starting with GDAL 2.2, there are 2 concepts: unset fields and null fields
// whereas previously there was only unset fields. For a GeoJSON output,
Expand Down
144 changes: 101 additions & 43 deletions src/providers/spatialite/qgsspatialiteprovider.cpp
View file
Open in desktop
Expand Up @@ -956,19 +956,22 @@ void QgsSpatiaLiteProvider::insertDefaultValue( int fieldIndex, QString defaultV

if ( mAttributeFields.at( fieldIndex ).name() != mPrimaryKey || ( mAttributeFields.at( fieldIndex ).name() == mPrimaryKey && !mPrimaryKeyAutoIncrement ) )
{
bool ok;
switch ( mAttributeFields.at( fieldIndex ).type() )
{
case QVariant::LongLong:
defaultVariant = defaultVal.toLongLong();
defaultVariant = defaultVal.toLongLong( &ok );
break;

case QVariant::Double:
defaultVariant = defaultVal.toDouble();
defaultVariant = defaultVal.toDouble( &ok );
break;

default:
{
if ( defaultVal.startsWith( '\'' ) )
// Literal string?
ok = defaultVal.startsWith( '\'' );
if ( ok )
defaultVal = defaultVal.remove( 0, 1 );
if ( defaultVal.endsWith( '\'' ) )
defaultVal.chop( 1 );
Expand All @@ -978,11 +981,52 @@ void QgsSpatiaLiteProvider::insertDefaultValue( int fieldIndex, QString defaultV
break;
}
}

if ( ! ok ) // Must be a SQL clause and not a literal
{
mDefaultValueClause.insert( fieldIndex, defaultVal );
}

}
mDefaultValues.insert( fieldIndex, defaultVariant );
mDefaultValues.insert( fieldIndex, defaultVal );
}
}


QVariant QgsSpatiaLiteProvider::defaultValue( int fieldId ) const
{
// TODO: backend-side evaluation
if ( fieldId < 0 || fieldId >= mAttributeFields.count() )
return QVariant();

QString defaultVal = mDefaultValues.value( fieldId, QString() );
if ( defaultVal.isEmpty() )
return QVariant();

QVariant resultVar = defaultVal;
if ( defaultVal == QStringLiteral( "CURRENT_TIMESTAMP" ) )
resultVar = QDateTime::currentDateTime();
else if ( defaultVal == QStringLiteral( "CURRENT_DATE" ) )
resultVar = QDate::currentDate();
else if ( defaultVal == QStringLiteral( "CURRENT_TIME" ) )
resultVar = QTime::currentTime();
else if ( defaultVal.startsWith( '\'' ) )
{
defaultVal = defaultVal.remove( 0, 1 );
defaultVal.chop( 1 );
defaultVal.replace( QLatin1String( "''" ), QLatin1String( "'" ) );
resultVar = defaultVal;
}

( void )mAttributeFields.at( fieldId ).convertCompatible( resultVar );
return resultVar;
}

QString QgsSpatiaLiteProvider::defaultValueClause( int fieldIndex ) const
{
return mDefaultValueClause.value( fieldIndex, QString() );
}

void QgsSpatiaLiteProvider::handleError( const QString &sql, char *errorMessage, bool rollback )
{
QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, errorMessage ? errorMessage : tr( "unknown cause" ) ), tr( "SpatiaLite" ) );
Expand Down Expand Up @@ -3957,10 +4001,11 @@ bool QgsSpatiaLiteProvider::addFeatures( QgsFeatureList &flist, Flags flags )
sqlite3_stmt *stmt = nullptr;
char *errMsg = nullptr;
bool toCommit = false;
QString sql;
QString values;
QString baseValues;
QString separator;
int ia, ret;
// SQL for single row
QString sql;

if ( flist.isEmpty() )
return true;
Expand All @@ -3971,46 +4016,58 @@ bool QgsSpatiaLiteProvider::addFeatures( QgsFeatureList &flist, Flags flags )
{
toCommit = true;

sql = QStringLiteral( "INSERT INTO %1(" ).arg( QgsSqliteUtils::quotedIdentifier( mTableName ) );
values = QStringLiteral( ") VALUES (" );
QString baseSql { QStringLiteral( "INSERT INTO %1(" ).arg( QgsSqliteUtils::quotedIdentifier( mTableName ) ) };
baseValues = QStringLiteral( ") VALUES (" );
separator.clear();

if ( !mGeometryColumn.isEmpty() )
{
sql += separator + QgsSqliteUtils::quotedIdentifier( mGeometryColumn );
values += separator + geomParam();
baseSql += separator + QgsSqliteUtils::quotedIdentifier( mGeometryColumn );
baseValues += separator + geomParam();
separator = ',';
}

for ( int i = 0; i < attributevec.count(); ++i )
for ( QgsFeatureList::iterator feature = flist.begin(); feature != flist.end(); ++feature )
{
if ( i >= mAttributeFields.count() )
continue;

QString fieldname = mAttributeFields.at( i ).name();
if ( fieldname.isEmpty() || fieldname == mGeometryColumn )
continue;
QString values { baseValues };
sql = baseSql;

sql += separator + QgsSqliteUtils::quotedIdentifier( fieldname );
values += separator + '?';
separator = ',';
}
// looping on each feature to insert
QgsAttributes attributevec = feature->attributes();

sql += values;
sql += ')';
// Default indexes (to be skipped)
QList<int> defaultIndexes;

// SQLite prepared statement
ret = sqlite3_prepare_v2( mSqliteHandle, sql.toUtf8().constData(), -1, &stmt, nullptr );
if ( ret == SQLITE_OK )
{
for ( QgsFeatureList::iterator feature = flist.begin(); feature != flist.end(); ++feature )
for ( int i = 0; i < attributevec.count(); ++i )
{
// looping on each feature to insert
QgsAttributes attributevec = feature->attributes();
if ( mDefaultValues.contains( i ) && mDefaultValues.value( i ) == attributevec.at( i ).toString() )
{
defaultIndexes.push_back( i );
continue;
}

// resetting Prepared Statement and bindings
sqlite3_reset( stmt );
sqlite3_clear_bindings( stmt );
if ( i >= mAttributeFields.count() )
continue;

QString fieldname = mAttributeFields.at( i ).name();
if ( fieldname.isEmpty() || fieldname == mGeometryColumn )
{
continue;
}

sql += separator + QgsSqliteUtils::quotedIdentifier( fieldname );
values += separator + '?';
separator = ',';
}

sql += values;
sql += ')';

// SQLite prepared statement
ret = sqlite3_prepare_v2( mSqliteHandle, sql.toUtf8().constData(), -1, &stmt, nullptr );
if ( ret == SQLITE_OK )
{

// initializing the column counter
ia = 0;
Expand Down Expand Up @@ -4039,6 +4096,11 @@ bool QgsSpatiaLiteProvider::addFeatures( QgsFeatureList &flist, Flags flags )

for ( int i = 0; i < attributevec.count(); ++i )
{
if ( defaultIndexes.contains( i ) )
{
continue;
}

QVariant v = attributevec.at( i );

// binding values for each attribute
Expand Down Expand Up @@ -4103,6 +4165,8 @@ bool QgsSpatiaLiteProvider::addFeatures( QgsFeatureList &flist, Flags flags )
// performing actual row insert
ret = sqlite3_step( stmt );

sqlite3_finalize( stmt );

if ( ret == SQLITE_DONE || ret == SQLITE_ROW )
{
// update feature id
Expand All @@ -4121,14 +4185,13 @@ bool QgsSpatiaLiteProvider::addFeatures( QgsFeatureList &flist, Flags flags )
break;
}
}
} // prepared statement

sqlite3_finalize( stmt );
if ( ret == SQLITE_DONE || ret == SQLITE_ROW )
{
ret = sqlite3_exec( mSqliteHandle, "COMMIT", nullptr, nullptr, &errMsg );
}

if ( ret == SQLITE_DONE || ret == SQLITE_ROW )
{
ret = sqlite3_exec( mSqliteHandle, "COMMIT", nullptr, nullptr, &errMsg );
}
} // prepared statement
} // BEGIN statement

if ( ret != SQLITE_OK )
Expand Down Expand Up @@ -4526,11 +4589,6 @@ QgsVectorDataProvider::Capabilities QgsSpatiaLiteProvider::capabilities() const
return mEnabledCapabilities;
}

QVariant QgsSpatiaLiteProvider::defaultValue( int fieldId ) const
{
return mDefaultValues.value( fieldId, QVariant() );
}

bool QgsSpatiaLiteProvider::skipConstraintCheck( int fieldIndex, QgsFieldConstraints::Constraint constraint, const QVariant &value ) const
{
Q_UNUSED( constraint )
Expand Down
8 changes: 7 additions & 1 deletion src/providers/spatialite/qgsspatialiteprovider.h
View file
Open in desktop
Expand Up @@ -221,6 +221,9 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider

QgsFields mAttributeFields;

//! Map of field index to default value SQL fragments
QMap<int, QString> mDefaultValueClause;

//! Flag indicating if the layer data source is a valid SpatiaLite layer
bool mValid = false;

Expand Down Expand Up @@ -264,7 +267,7 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider
QString mGeometryColumn;

//! Map of field index to default value
QMap<int, QVariant> mDefaultValues;
QMap<int, QString> mDefaultValues;

//! Name of the SpatialIndex table
QString mIndexTable;
Expand Down Expand Up @@ -382,6 +385,9 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider

friend class QgsSpatiaLiteFeatureSource;

// QgsVectorDataProvider interface
public:
virtual QString defaultValueClause( int fieldIndex ) const override;
};

class QgsSpatiaLiteProviderMetadata: public QgsProviderMetadata
Expand Down