Skip to content

Commit

Permalink
Invalid join cache when layer is modified (fix #11140)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nyalldawson
nyalldawson committed Jun 13, 2016
1 parent 1ec7ad5 commit 0a5ad73
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 2 deletions.
3 changes: 3 additions & 0 deletions python/core/qgsvectorlayer.sip
View file
Expand Up @@ -8,6 +8,7 @@ struct QgsVectorJoinInfo
%TypeHeaderCode
#include "qgsvectorlayer.h"
%End
QgsVectorJoinInfo();

/** Join field in the target layer*/
QString targetFieldName;
Expand All @@ -17,6 +18,8 @@ struct QgsVectorJoinInfo
QString joinFieldName;
/** True if the join is cached in virtual memory*/
bool memoryCache;
/** True if the cached join attributes need to be updated*/
bool cacheDirty;
/** Cache for joined attributes to provide fast lookup (size is 0 if no memory caching)
* @note not available in python bindings
*/
Expand Down
10 changes: 10 additions & 0 deletions src/core/qgsvectorlayer.h
View file
Expand Up @@ -75,6 +75,13 @@ typedef QList<QgsPointV2> QgsPointSequenceV2;

struct CORE_EXPORT QgsVectorJoinInfo
{
QgsVectorJoinInfo()
: memoryCache( false )
, cacheDirty( true )
, targetFieldIndex( -1 )
, joinFieldIndex( -1 )
{}

/** Join field in the target layer*/
QString targetFieldName;
/** Source layer*/
Expand All @@ -83,6 +90,9 @@ struct CORE_EXPORT QgsVectorJoinInfo
QString joinFieldName;
/** True if the join is cached in virtual memory*/
bool memoryCache;
/** True if the cached join attributes need to be updated*/
bool cacheDirty;

/** Cache for joined attributes to provide fast lookup (size is 0 if no memory caching)
* @note not available in python bindings
*/
Expand Down
3 changes: 3 additions & 0 deletions src/core/qgsvectorlayerfeatureiterator.cpp
View file
Expand Up @@ -31,7 +31,10 @@ QgsVectorLayerFeatureSource::QgsVectorLayerFeatureSource( QgsVectorLayer *layer
{
mProviderFeatureSource = layer->dataProvider()->featureSource();
mFields = layer->fields();

layer->createJoinCaches();
mJoinBuffer = layer->mJoinBuffer->clone();

mExpressionFieldBuffer = new QgsExpressionFieldBuffer( *layer->mExpressionFieldBuffer );
mCrsId = layer->crs().srsid();

Expand Down
29 changes: 27 additions & 2 deletions src/core/qgsvectorlayerjoinbuffer.cpp
View file
Expand Up @@ -87,6 +87,7 @@ bool QgsVectorLayerJoinBuffer::addJoin( const QgsVectorJoinInfo& joinInfo )
if ( QgsVectorLayer* vl = qobject_cast<QgsVectorLayer*>( QgsMapLayerRegistry::instance()->mapLayer( joinInfo.joinLayerId ) ) )
{
connect( vl, SIGNAL( updatedFields() ), this, SLOT( joinedLayerUpdatedFields() ), Qt::UniqueConnection );
connect( vl, SIGNAL( layerModified() ), this, SLOT( joinedLayerModified() ), Qt::UniqueConnection );
}

emit joinedFieldsChanged();
Expand Down Expand Up @@ -118,7 +119,7 @@ bool QgsVectorLayerJoinBuffer::removeJoin( const QString& joinLayerId )
void QgsVectorLayerJoinBuffer::cacheJoinLayer( QgsVectorJoinInfo& joinInfo )
{
//memory cache not required or already done
if ( !joinInfo.memoryCache || !joinInfo.cachedAttributes.isEmpty() )
if ( !joinInfo.memoryCache || !joinInfo.cacheDirty )
{
return;
}
Expand Down Expand Up @@ -175,6 +176,7 @@ void QgsVectorLayerJoinBuffer::cacheJoinLayer( QgsVectorJoinInfo& joinInfo )
joinInfo.cachedAttributes.insert( key, attrs2 );
}
}
joinInfo.cacheDirty = false;
}
}

Expand Down Expand Up @@ -260,11 +262,15 @@ void QgsVectorLayerJoinBuffer::createJoinCaches()
QList< QgsVectorJoinInfo >::iterator joinIt = mVectorJoins.begin();
for ( ; joinIt != mVectorJoins.end(); ++joinIt )
{
cacheJoinLayer( *joinIt );
if ( joinIt->memoryCache && joinIt->cacheDirty )

This comment has been minimized.

Sign in to view

Copy link
@3nids

3nids Jun 16, 2016

Member

I get a crash here when opening an atlas composer
here is the stacktrace

image

do you need further details?

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Jun 16, 2016

Author Collaborator

Any chance you could share a sample project?

This comment has been minimized.

Sign in to view

Copy link
@3nids

3nids Jun 16, 2016

Member

not easily.

This comment has been minimized.

Sign in to view

Copy link
@3nids

3nids Jun 16, 2016

Member

would you like to do a remote desktop connection so you can have a look? chat on hangouts?

This comment has been minimized.

Sign in to view

Copy link
@3nids

3nids Jun 20, 2016

Member

@nyalldawson any news on this?

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Jun 20, 2016

Author Collaborator

@3nids haven't had a chance to dig into it yet - I'm mid way through a house move, so only have the brainpower for small stuff until Friday :)

This comment has been minimized.

Sign in to view

Copy link
@3nids

3nids Jun 20, 2016

Member

@m-kuhn had a look and proposed this #3224
thanks a lot and all the best for the moving in....
cacheJoinLayer( *joinIt );

// make sure we are connected to the joined layer
if ( QgsVectorLayer* vl = qobject_cast<QgsVectorLayer*>( QgsMapLayerRegistry::instance()->mapLayer( joinIt->joinLayerId ) ) )
{
connect( vl, SIGNAL( updatedFields() ), this, SLOT( joinedLayerUpdatedFields() ), Qt::UniqueConnection );
connect( vl, SIGNAL( layerModified() ), this, SLOT( joinedLayerModified() ), Qt::UniqueConnection );
}
}
}

Expand Down Expand Up @@ -329,6 +335,7 @@ void QgsVectorLayerJoinBuffer::readXml( const QDomNode& layer_node )
info.joinLayerId = infoElem.attribute( "joinLayerId" );
info.targetFieldName = infoElem.attribute( "targetFieldName" );
info.memoryCache = infoElem.attribute( "memoryCache" ).toInt();
info.cacheDirty = true;

info.joinFieldIndex = infoElem.attribute( "joinField" ).toInt(); //for compatibility with 1.x
info.targetFieldIndex = infoElem.attribute( "targetField" ).toInt(); //for compatibility with 1.x
Expand Down Expand Up @@ -397,6 +404,9 @@ QgsVectorLayerJoinBuffer* QgsVectorLayerJoinBuffer::clone() const

void QgsVectorLayerJoinBuffer::joinedLayerUpdatedFields()
{
// TODO - check - this whole method is probably not needed anymore,
// since the cache handling is covered by joinedLayerModified()

QgsVectorLayer* joinedLayer = qobject_cast<QgsVectorLayer*>( sender() );
Q_ASSERT( joinedLayer );

Expand All @@ -412,3 +422,18 @@ void QgsVectorLayerJoinBuffer::joinedLayerUpdatedFields()

emit joinedFieldsChanged();
}

void QgsVectorLayerJoinBuffer::joinedLayerModified()
{
QgsVectorLayer* joinedLayer = qobject_cast<QgsVectorLayer*>( sender() );
Q_ASSERT( joinedLayer );

// recache the joined layer
for ( QgsVectorJoinList::iterator it = mVectorJoins.begin(); it != mVectorJoins.end(); ++it )
{
if ( joinedLayer->id() == it->joinLayerId )
{
it->cacheDirty = true;
}
}
}
2 changes: 2 additions & 0 deletions src/core/qgsvectorlayerjoinbuffer.h
View file
Expand Up @@ -90,6 +90,8 @@ class CORE_EXPORT QgsVectorLayerJoinBuffer : public QObject
private slots:
void joinedLayerUpdatedFields();

void joinedLayerModified();

private:

QgsVectorLayer* mLayer;
Expand Down
83 changes: 83 additions & 0 deletions tests/src/core/testqgsvectorlayerjoinbuffer.cpp
View file
Expand Up @@ -57,6 +57,8 @@ class TestVectorLayerJoinBuffer : public QObject
void testJoinTwoTimes_data();
void testJoinTwoTimes();
void testJoinLayerDefinitionFile();
void testCacheUpdate_data();
void testCacheUpdate();

private:
QList<QString> mProviders;
Expand Down Expand Up @@ -519,6 +521,87 @@ void TestVectorLayerJoinBuffer::testJoinLayerDefinitionFile()
QVERIFY( vLayer->fieldNameIndex( joinInfo.prefix + "value" ) >= 0 );
}

void TestVectorLayerJoinBuffer::testCacheUpdate_data()
{
QTest::addColumn<bool>( "useCache" );
QTest::newRow( "cache" ) << true;
QTest::newRow( "no cache" ) << false;
}

void TestVectorLayerJoinBuffer::testCacheUpdate()
{
QFETCH( bool, useCache );

QgsVectorLayer* vlA = new QgsVectorLayer( "Point?field=id_a:integer", "cacheA", "memory" );
QVERIFY( vlA->isValid() );
QgsVectorLayer* vlB = new QgsVectorLayer( "Point?field=id_b:integer&field=value_b", "cacheB", "memory" );
QVERIFY( vlB->isValid() );
QgsMapLayerRegistry::instance()->addMapLayer( vlA );
QgsMapLayerRegistry::instance()->addMapLayer( vlB );

QgsFeature fA1( vlA->dataProvider()->fields(), 1 );
fA1.setAttribute( "id_a", 1 );
QgsFeature fA2( vlA->dataProvider()->fields(), 2 );
fA2.setAttribute( "id_a", 2 );

vlA->dataProvider()->addFeatures( QgsFeatureList() << fA1 << fA2 );

QgsFeature fB1( vlB->dataProvider()->fields(), 1 );
fB1.setAttribute( "id_b", 1 );
fB1.setAttribute( "value_b", 11 );
QgsFeature fB2( vlB->dataProvider()->fields(), 2 );
fB2.setAttribute( "id_b", 2 );
fB2.setAttribute( "value_b", 12 );

vlB->dataProvider()->addFeatures( QgsFeatureList() << fB1 << fB2 );

QgsVectorJoinInfo joinInfo;
joinInfo.targetFieldName = "id_a";
joinInfo.joinLayerId = vlB->id();
joinInfo.joinFieldName = "id_b";
joinInfo.memoryCache = useCache;
joinInfo.prefix = "B_";
vlA->addJoin( joinInfo );

QgsFeatureIterator fi = vlA->getFeatures();
fi.nextFeature( fA1 );
QCOMPARE( fA1.attribute( "id_a" ).toInt(), 1 );
QCOMPARE( fA1.attribute( "B_value_b" ).toInt(), 11 );
fi.nextFeature( fA2 );
QCOMPARE( fA2.attribute( "id_a" ).toInt(), 2 );
QCOMPARE( fA2.attribute( "B_value_b" ).toInt(), 12 );

// change value in join target layer
vlB->startEditing();
vlB->changeAttributeValue( 1, 1, 111 );
vlB->changeAttributeValue( 2, 0, 3 );
vlB->commitChanges();

fi = vlA->getFeatures();
fi.nextFeature( fA1 );
QCOMPARE( fA1.attribute( "id_a" ).toInt(), 1 );
QCOMPARE( fA1.attribute( "B_value_b" ).toInt(), 111 );
fi.nextFeature( fA2 );
QCOMPARE( fA2.attribute( "id_a" ).toInt(), 2 );
QVERIFY( fA2.attribute( "B_value_b" ).isNull() );

// change value in joined layer
vlA->startEditing();
vlA->changeAttributeValue( 2, 0, 3 );
vlA->commitChanges();

fi = vlA->getFeatures();
fi.nextFeature( fA1 );
QCOMPARE( fA1.attribute( "id_a" ).toInt(), 1 );
QCOMPARE( fA1.attribute( "B_value_b" ).toInt(), 111 );
fi.nextFeature( fA2 );
QCOMPARE( fA2.attribute( "id_a" ).toInt(), 3 );
QCOMPARE( fA2.attribute( "B_value_b" ).toInt(), 12 );

QgsMapLayerRegistry::instance()->removeMapLayer( vlA );
QgsMapLayerRegistry::instance()->removeMapLayer( vlB );
}


QTEST_MAIN( TestVectorLayerJoinBuffer )
#include "testqgsvectorlayerjoinbuffer.moc"
Expand Down

2 comments on commit 0a5ad73

@3nids
Copy link
Member

@3nids 3nids commented on 0a5ad73 Jun 13, 2016

Choose a reason for hiding this comment

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

oh yes, finally!

@nirvn
Copy link
Contributor

@nirvn nirvn commented on 0a5ad73 Jun 13, 2016

Choose a reason for hiding this comment

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

I second that, huge improvement for anyone dealing with joins.

Please sign in to comment.