Skip to content

Commit 9d3f8d4

Browse files
rouaultrouault
authoredJun 1, 2018
Merge pull request #7140 from rouault/fix_19009
Assorted set of fixes regarding field length for OGR provider
·
ltr-3_40final-3_2_0
2 parents 3b29102 + b9003ff commit 9d3f8d4

File tree

5 files changed

+134
-59
lines changed

5 files changed

+134
-59
lines changed
 

‎src/app/qgsfieldcalculator.cpp

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,15 @@
2828

2929
#include <QMessageBox>
3030

31+
// FTC = FieldTypeCombo
32+
constexpr int FTC_TYPE_ROLE_IDX = 0;
33+
constexpr int FTC_TYPE_NAME_IDX = 1;
34+
constexpr int FTC_MINLEN_IDX = 2;
35+
constexpr int FTC_MAXLEN_IDX = 3;
36+
constexpr int FTC_MINPREC_IDX = 4;
37+
constexpr int FTC_MAXPREC_IDX = 5;
38+
constexpr int FTC_SUBTYPE_IDX = 6;
39+
3140
QgsFieldCalculator::QgsFieldCalculator( QgsVectorLayer *vl, QWidget *parent )
3241
: QDialog( parent )
3342
, mVectorLayer( vl )
@@ -338,13 +347,13 @@ void QgsFieldCalculator::populateOutputFieldTypes()
338347
for ( int i = 0; i < typelist.size(); i++ )
339348
{
340349
mOutputFieldTypeComboBox->addItem( typelist[i].mTypeDesc );
341-
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mType ), Qt::UserRole );
342-
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mTypeName, Qt::UserRole + 1 );
343-
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinLen, Qt::UserRole + 2 );
344-
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxLen, Qt::UserRole + 3 );
345-
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinPrec, Qt::UserRole + 4 );
346-
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxPrec, Qt::UserRole + 5 );
347-
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mSubType ), Qt::UserRole + 6 );
350+
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mType ), Qt::UserRole + FTC_TYPE_ROLE_IDX );
351+
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mTypeName, Qt::UserRole + FTC_TYPE_NAME_IDX );
352+
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinLen, Qt::UserRole + FTC_MINLEN_IDX );
353+
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxLen, Qt::UserRole + FTC_MAXLEN_IDX );
354+
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinPrec, Qt::UserRole + FTC_MINPREC_IDX );
355+
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxPrec, Qt::UserRole + FTC_MAXPREC_IDX );
356+
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mSubType ), Qt::UserRole + FTC_SUBTYPE_IDX );
348357
}
349358
mOutputFieldTypeComboBox->blockSignals( false );
350359
mOutputFieldTypeComboBox->setCurrentIndex( 0 );
@@ -416,8 +425,8 @@ void QgsFieldCalculator::mOutputFieldNameLineEdit_textChanged( const QString &te
416425

417426
void QgsFieldCalculator::mOutputFieldTypeComboBox_activated( int index )
418427
{
419-
mOutputFieldWidthSpinBox->setMinimum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + 2 ).toInt() );
420-
mOutputFieldWidthSpinBox->setMaximum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + 3 ).toInt() );
428+
mOutputFieldWidthSpinBox->setMinimum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + FTC_MINLEN_IDX ).toInt() );
429+
mOutputFieldWidthSpinBox->setMaximum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + FTC_MAXLEN_IDX ).toInt() );
421430
mOutputFieldWidthSpinBox->setEnabled( mOutputFieldWidthSpinBox->minimum() < mOutputFieldWidthSpinBox->maximum() );
422431
if ( mOutputFieldWidthSpinBox->value() < mOutputFieldWidthSpinBox->minimum() )
423432
mOutputFieldWidthSpinBox->setValue( mOutputFieldWidthSpinBox->minimum() );
@@ -482,8 +491,8 @@ void QgsFieldCalculator::setOkButtonState()
482491
void QgsFieldCalculator::setPrecisionMinMax()
483492
{
484493
int idx = mOutputFieldTypeComboBox->currentIndex();
485-
int minPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + 4 ).toInt();
486-
int maxPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + 5 ).toInt();
494+
int minPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + FTC_MINPREC_IDX ).toInt();
495+
int maxPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + FTC_MAXPREC_IDX ).toInt();
487496
mOutputFieldPrecisionSpinBox->setEnabled( minPrecType < maxPrecType );
488497
mOutputFieldPrecisionSpinBox->setMinimum( minPrecType );
489498
mOutputFieldPrecisionSpinBox->setMaximum( std::max( minPrecType, std::min( maxPrecType, mOutputFieldWidthSpinBox->value() ) ) );
@@ -493,3 +502,15 @@ void QgsFieldCalculator::showHelp()
493502
{
494503
QgsHelp::openHelp( QStringLiteral( "working_with_vector/attribute_table.html#editing-attribute-values" ) );
495504
}
505+
506+
QgsField QgsFieldCalculator::fieldDefinition()
507+
{
508+
return QgsField( mOutputFieldNameLineEdit->text(),
509+
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole + FTC_TYPE_ROLE_IDX ).toInt() ),
510+
mOutputFieldTypeComboBox->currentData( Qt::UserRole + FTC_TYPE_NAME_IDX ).toString(),
511+
mOutputFieldWidthSpinBox->value(),
512+
mOutputFieldPrecisionSpinBox->value(),
513+
QString(),
514+
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole + FTC_SUBTYPE_IDX ).toInt() )
515+
);
516+
}

‎src/app/qgsfieldcalculator.h

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -61,17 +61,7 @@ class APP_EXPORT QgsFieldCalculator: public QDialog, private Ui::QgsFieldCalcula
6161
QMap<QString, int> mFieldMap;
6262

6363
//! Create a field based on the definitions
64-
inline QgsField fieldDefinition()
65-
{
66-
return QgsField( mOutputFieldNameLineEdit->text(),
67-
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole ).toInt() ),
68-
mOutputFieldTypeComboBox->currentData( Qt::UserRole + 1 ).toString(),
69-
mOutputFieldWidthSpinBox->value(),
70-
mOutputFieldPrecisionSpinBox->value(),
71-
QString(),
72-
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole + 6 ).toInt() )
73-
);
74-
}
64+
QgsField fieldDefinition();
7565

7666
//! Idx of changed attribute
7767
int mAttributeId;

‎src/core/qgsvectordataprovider.cpp

Lines changed: 7 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -343,40 +343,22 @@ bool QgsVectorDataProvider::supportedType( const QgsField &field ) const
343343
if ( field.type() != nativeType.mType )
344344
continue;
345345

346-
if ( field.length() == -1 )
347-
{
348-
// source length unlimited
349-
if ( nativeType.mMinLen > -1 || nativeType.mMaxLen > -1 )
350-
{
351-
// destination limited
352-
continue;
353-
}
354-
}
355-
else
346+
if ( field.length() > 0 )
356347
{
357348
// source length limited
358-
if ( nativeType.mMinLen > -1 && nativeType.mMaxLen > -1 &&
359-
( field.length() < nativeType.mMinLen || field.length() > nativeType.mMaxLen ) )
349+
if ( ( nativeType.mMinLen > 0 && field.length() < nativeType.mMinLen ) ||
350+
( nativeType.mMaxLen > 0 && field.length() > nativeType.mMaxLen ) )
360351
{
361352
// source length exceeds destination limits
362353
continue;
363354
}
364355
}
365356

366-
if ( field.precision() == -1 )
367-
{
368-
// source precision unlimited / n/a
369-
if ( nativeType.mMinPrec > -1 || nativeType.mMaxPrec > -1 )
370-
{
371-
// destination limited
372-
continue;
373-
}
374-
}
375-
else
357+
if ( field.precision() > 0 )
376358
{
377-
// source precision unlimited / n/a
378-
if ( nativeType.mMinPrec > -1 && nativeType.mMaxPrec > -1 &&
379-
( field.precision() < nativeType.mMinPrec || field.precision() > nativeType.mMaxPrec ) )
359+
// source precision limited
360+
if ( ( nativeType.mMinPrec > 0 && field.precision() < nativeType.mMinPrec ) ||
361+
( nativeType.mMaxPrec > 0 && field.precision() > nativeType.mMaxPrec ) )
380362
{
381363
// source precision exceeds destination limits
382364
continue;

‎src/providers/ogr/qgsogrprovider.cpp

Lines changed: 74 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -449,20 +449,60 @@ QgsOgrProvider::QgsOgrProvider( QString const &uri, const ProviderOptions &optio
449449
450450
open( OpenModeInitial );
451451
452+
int nMaxIntLen = 11;
453+
int nMaxInt64Len = 21;
454+
int nMaxDoubleLen = 20;
455+
int nMaxDoublePrec = 15;
456+
int nDateLen = 8;
457+
if ( mGDALDriverName == QLatin1String( "GPKG" ) )
458+
{
459+
// GPKG only supports field length for text (and binary)
460+
nMaxIntLen = 0;
461+
nMaxInt64Len = 0;
462+
nMaxDoubleLen = 0;
463+
nMaxDoublePrec = 0;
464+
nDateLen = 0;
465+
}
466+
452467
QList<NativeType> nativeTypes;
453468
nativeTypes
454-
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer)" ), QStringLiteral( "integer" ), QVariant::Int, 0, 11 )
455-
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer 64 bit)" ), QStringLiteral( "integer64" ), QVariant::LongLong, 0, 21 )
456-
<< QgsVectorDataProvider::NativeType( tr( "Decimal number (real)" ), QStringLiteral( "double" ), QVariant::Double, 0, 20, 0, 15 )
457-
<< QgsVectorDataProvider::NativeType( tr( "Text (string)" ), QStringLiteral( "string" ), QVariant::String, 0, 65535 )
458-
<< QgsVectorDataProvider::NativeType( tr( "Date" ), QStringLiteral( "date" ), QVariant::Date, 8, 8 );
469+
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer)" ), QStringLiteral( "integer" ), QVariant::Int, 0, nMaxIntLen )
470+
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer 64 bit)" ), QStringLiteral( "integer64" ), QVariant::LongLong, 0, nMaxInt64Len )
471+
<< QgsVectorDataProvider::NativeType( tr( "Decimal number (real)" ), QStringLiteral( "double" ), QVariant::Double, 0, nMaxDoubleLen, 0, nMaxDoublePrec )
472+
<< QgsVectorDataProvider::NativeType( tr( "Text (string)" ), QStringLiteral( "string" ), QVariant::String, 0, 65535 );
473+
474+
bool supportsDate = true;
475+
bool supportsTime = mGDALDriverName != QLatin1String( "ESRI Shapefile" ) && mGDALDriverName != QLatin1String( "GPKG" );
476+
bool supportsDateTime = mGDALDriverName != QLatin1String( "ESRI Shapefile" );
477+
const char *pszDataTypes = nullptr;
478+
if ( mOgrOrigLayer )
479+
{
480+
pszDataTypes = GDALGetMetadataItem( mOgrOrigLayer->driver(), GDAL_DMD_CREATIONFIELDDATATYPES, nullptr );
481+
}
482+
// For drivers that advertize their data type, use that instead of the
483+
// above hardcoded defaults.
484+
if ( pszDataTypes )
485+
{
486+
char **papszTokens = CSLTokenizeString2( pszDataTypes, " ", 0 );
487+
supportsDate = CSLFindString( papszTokens, "Date" ) >= 0;
488+
supportsTime = CSLFindString( papszTokens, "Time" ) >= 0;
489+
supportsDateTime = CSLFindString( papszTokens, "DateTime" ) >= 0;
490+
CSLDestroy( papszTokens );
491+
}
459492

460-
// Some drivers do not support datetime type
461-
// Please help to fill this list
462-
if ( mGDALDriverName != QLatin1String( "ESRI Shapefile" ) )
493+
if ( supportsDate )
494+
{
495+
nativeTypes
496+
<< QgsVectorDataProvider::NativeType( tr( "Date" ), QStringLiteral( "date" ), QVariant::Date, nDateLen, nDateLen );
497+
}
498+
if ( supportsTime )
499+
{
500+
nativeTypes
501+
<< QgsVectorDataProvider::NativeType( tr( "Time" ), QStringLiteral( "time" ), QVariant::Time );
502+
}
503+
if ( supportsDateTime )
463504
{
464505
nativeTypes
465-
<< QgsVectorDataProvider::NativeType( tr( "Time" ), QStringLiteral( "time" ), QVariant::Time, -1, -1 )
466506
<< QgsVectorDataProvider::NativeType( tr( "Date & Time" ), QStringLiteral( "datetime" ), QVariant::DateTime );
467507
}
468508

@@ -471,8 +511,8 @@ QgsOgrProvider::QgsOgrProvider( QString const &uri, const ProviderOptions &optio
471511
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,3,0)
472512
if ( mOgrOrigLayer )
473513
{
474-
const char *pszDataTypes = GDALGetMetadataItem( mOgrOrigLayer->driver(), GDAL_DMD_CREATIONFIELDDATASUBTYPES, nullptr );
475-
if ( pszDataTypes && strstr( pszDataTypes, "Boolean" ) )
514+
const char *pszDataSubTypes = GDALGetMetadataItem( mOgrOrigLayer->driver(), GDAL_DMD_CREATIONFIELDDATASUBTYPES, nullptr );
515+
if ( pszDataSubTypes && strstr( pszDataSubTypes, "Boolean" ) )
476516
supportsBoolean = true;
477517
}
478518
#else
@@ -492,7 +532,7 @@ QgsOgrProvider::QgsOgrProvider( QString const &uri, const ProviderOptions &optio
492532
{
493533
// boolean data type
494534
nativeTypes
495-
<< QgsVectorDataProvider::NativeType( tr( "Boolean" ), QStringLiteral( "bool" ), QVariant::Bool, -1, -1, -1, -1 );
535+
<< QgsVectorDataProvider::NativeType( tr( "Boolean" ), QStringLiteral( "bool" ), QVariant::Bool );
496536
}
497537

498538
setNativeTypes( nativeTypes );
@@ -1567,6 +1607,10 @@ bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
15671607
returnvalue = false;
15681608
}
15691609
}
1610+
1611+
// Backup existing fields. We need them to 'restore' field type, length, precision
1612+
QgsFields oldFields = mAttributeFields;
1613+
15701614
loadFields();
15711615

15721616
// The check in QgsVectorLayerEditBuffer::commitChanges() is questionable with
@@ -1588,6 +1632,24 @@ bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
15881632
}
15891633
}
15901634

1635+
// Restore field type, length, precision of existing fields as well
1636+
// We need that in scenarios where the user adds a int field with length != 0
1637+
// in a editing session, and repeat that again in another editing session
1638+
// Without the below hack, the length of the first added field would have
1639+
// been reset to zero, and QgsVectorLayerEditBuffer::commitChanges() would
1640+
// error out because of this.
1641+
// See https://issues.qgis.org/issues/19009
1642+
for ( auto field : oldFields )
1643+
{
1644+
int idx = mAttributeFields.lookupField( field.name() );
1645+
if ( idx >= 0 )
1646+
{
1647+
mAttributeFields[ idx ].setType( field.type() );
1648+
mAttributeFields[ idx ].setLength( field.length() );
1649+
mAttributeFields[ idx ].setPrecision( field.precision() );
1650+
}
1651+
}
1652+
15911653
return returnvalue;
15921654
}
15931655

‎tests/src/python/test_provider_ogr_gpkg.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -929,6 +929,26 @@ def testRequestWithoutGeometryOnLayerMixedGeometry(self):
929929
features = [f for f in vl.getFeatures(request)]
930930
self.assertEqual(len(features), 1)
931931

932+
def testAddingTwoIntFieldsWithWidth(self):
933+
""" Test buggfix for https://issues.qgis.org/issues/19009 """
934+
935+
tmpfile = os.path.join(self.basetestpath, 'testRequestWithoutGeometryOnLayerMixedGeometry.gpkg')
936+
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
937+
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint, options=['SPATIAL_INDEX=NO'])
938+
lyr.CreateField(ogr.FieldDefn('a', ogr.OFTInteger))
939+
ds = None
940+
941+
vl = QgsVectorLayer(u'{}'.format(tmpfile) + "|layername=" + "test", 'test', u'ogr')
942+
self.assertTrue(vl.isValid())
943+
944+
vl.startEditing()
945+
self.assertTrue(vl.addAttribute(QgsField("b", QVariant.Int, "integer", 10)))
946+
self.assertTrue(vl.commitChanges())
947+
948+
vl.startEditing()
949+
self.assertTrue(vl.addAttribute(QgsField("c", QVariant.Int, "integer", 10)))
950+
self.assertTrue(vl.commitChanges())
951+
932952

933953
if __name__ == '__main__':
934954
unittest.main()

0 commit comments

Comments
 (0)
Please sign in to comment.