Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit dc58e15

Browse files
strknyalldawson
strk
authored and
nyalldawson
committedMar 5, 2023
Make SRID/CRS cache a per-connection object, rather than a process-static
Static cache is wrong (same SRID may have different meaning in different databases) and dangerous (can result in segfaults). Closes GH-51893 Includes simple testcase for EWKT input/output in testqgspostgresprovider.cpp and allow skipping it by setting QGIS_PGTEST_DB_SKIP env variable
1 parent aa84c0b commit dc58e15

File tree

5 files changed

+132
-62
lines changed

5 files changed

+132
-62
lines changed
 

‎src/providers/postgres/qgspostgresconn.cpp

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2781,3 +2781,65 @@ QString QgsPostgresConn::currentDatabase() const
27812781

27822782
return database;
27832783
}
2784+
2785+
QgsCoordinateReferenceSystem QgsPostgresConn::sridToCrs( int srid )
2786+
{
2787+
QgsCoordinateReferenceSystem crs;
2788+
2789+
QMutexLocker locker( &mCrsCacheMutex );
2790+
if ( mCrsCache.contains( srid ) )
2791+
crs = mCrsCache.value( srid );
2792+
else
2793+
{
2794+
QgsPostgresResult result( LoggedPQexec( QStringLiteral( "QgsPostgresProvider" ), QStringLiteral( "SELECT auth_name, auth_srid, srtext, proj4text FROM spatial_ref_sys WHERE srid=%1" ).arg( srid ) ) );
2795+
if ( result.PQresultStatus() == PGRES_TUPLES_OK )
2796+
{
2797+
if ( result.PQntuples() > 0 )
2798+
{
2799+
const QString authName = result.PQgetvalue( 0, 0 );
2800+
const QString authSRID = result.PQgetvalue( 0, 1 );
2801+
const QString srText = result.PQgetvalue( 0, 2 );
2802+
bool ok = false;
2803+
if ( authName == QLatin1String( "EPSG" ) || authName == QLatin1String( "ESRI" ) )
2804+
{
2805+
ok = crs.createFromUserInput( authName + ':' + authSRID );
2806+
}
2807+
if ( !ok && !srText.isEmpty() )
2808+
{
2809+
ok = crs.createFromUserInput( srText );
2810+
}
2811+
if ( !ok )
2812+
crs = QgsCoordinateReferenceSystem::fromProj( result.PQgetvalue( 0, 3 ) );
2813+
}
2814+
mCrsCache.insert( srid, crs );
2815+
}
2816+
}
2817+
return crs;
2818+
}
2819+
2820+
int QgsPostgresConn::crsToSrid( const QgsCoordinateReferenceSystem &crs )
2821+
{
2822+
QMutexLocker locker( &mCrsCacheMutex );
2823+
int srid = mCrsCache.key( crs );
2824+
2825+
if ( srid > -1 )
2826+
return srid;
2827+
else
2828+
{
2829+
QStringList authParts = crs.authid().split( ':' );
2830+
if ( authParts.size() != 2 )
2831+
return -1;
2832+
const QString authName = authParts.first();
2833+
const QString authId = authParts.last();
2834+
QgsPostgresResult result( PQexec( QStringLiteral( "SELECT srid FROM spatial_ref_sys WHERE auth_name='%1' AND auth_srid=%2" ).arg( authName, authId ) ) );
2835+
2836+
if ( result.PQresultStatus() == PGRES_TUPLES_OK )
2837+
{
2838+
int srid = result.PQgetvalue( 0, 0 ).toInt();
2839+
mCrsCache.insert( srid, crs );
2840+
return srid;
2841+
}
2842+
}
2843+
2844+
return -1;
2845+
}

‎src/providers/postgres/qgspostgresconn.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,10 @@ class QgsPostgresConn : public QObject
456456
void lock() { mLock.lock(); }
457457
void unlock() { mLock.unlock(); }
458458

459+
QgsCoordinateReferenceSystem sridToCrs( int srsId );
460+
461+
int crsToSrid( const QgsCoordinateReferenceSystem &crs );
462+
459463
private:
460464

461465
int mRef;
@@ -529,6 +533,12 @@ class QgsPostgresConn : public QObject
529533
bool mTransaction;
530534

531535
mutable QRecursiveMutex mLock;
536+
537+
/* Mutex protecting sCrsCache */
538+
QMutex mCrsCacheMutex;
539+
540+
/* Cache of SRID to CRS */
541+
mutable QMap<int, QgsCoordinateReferenceSystem> mCrsCache;
532542
};
533543

534544
// clazy:excludeall=qstring-allocations

‎src/providers/postgres/qgspostgresprovider.cpp

Lines changed: 4 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -446,74 +446,17 @@ QString QgsPostgresProvider::geomAttrToString( const QVariant &attr, QgsPostgres
446446
return toEwkt( attr.value<QgsReferencedGeometry>(), conn );
447447
}
448448

449-
static QMutex sMutex;
450-
static QMap<int, QgsCoordinateReferenceSystem> sCrsCache;
451-
452449
int QgsPostgresProvider::crsToSrid( const QgsCoordinateReferenceSystem &crs, QgsPostgresConn *conn )
453450
{
454-
QMutexLocker locker( &sMutex );
455-
int srid = sCrsCache.key( crs );
456-
457-
if ( srid > -1 )
458-
return srid;
459-
else
460-
{
461-
if ( conn )
462-
{
463-
QStringList authParts = crs.authid().split( ':' );
464-
if ( authParts.size() != 2 )
465-
return -1;
466-
const QString authName = authParts.first();
467-
const QString authId = authParts.last();
468-
QgsPostgresResult result( conn->PQexec( QStringLiteral( "SELECT srid FROM spatial_ref_sys WHERE auth_name='%1' AND auth_srid=%2" ).arg( authName, authId ) ) );
469-
470-
if ( result.PQresultStatus() == PGRES_TUPLES_OK )
471-
{
472-
int srid = result.PQgetvalue( 0, 0 ).toInt();
473-
sCrsCache.insert( srid, crs );
474-
return srid;
475-
}
476-
}
477-
}
478-
479-
return -1;
451+
int srid = -1;
452+
if ( conn ) srid = conn->crsToSrid( crs );
453+
return srid;
480454
}
481455

482456
QgsCoordinateReferenceSystem QgsPostgresProvider::sridToCrs( int srid, QgsPostgresConn *conn )
483457
{
484458
QgsCoordinateReferenceSystem crs;
485-
486-
QMutexLocker locker( &sMutex );
487-
if ( sCrsCache.contains( srid ) )
488-
crs = sCrsCache.value( srid );
489-
else
490-
{
491-
if ( conn )
492-
{
493-
QgsPostgresResult result( conn->LoggedPQexec( QStringLiteral( "QgsPostgresProvider" ), QStringLiteral( "SELECT auth_name, auth_srid, srtext, proj4text FROM spatial_ref_sys WHERE srid=%1" ).arg( srid ) ) );
494-
if ( result.PQresultStatus() == PGRES_TUPLES_OK )
495-
{
496-
if ( result.PQntuples() > 0 )
497-
{
498-
const QString authName = result.PQgetvalue( 0, 0 );
499-
const QString authSRID = result.PQgetvalue( 0, 1 );
500-
const QString srText = result.PQgetvalue( 0, 2 );
501-
bool ok = false;
502-
if ( authName == QLatin1String( "EPSG" ) || authName == QLatin1String( "ESRI" ) )
503-
{
504-
ok = crs.createFromUserInput( authName + ':' + authSRID );
505-
}
506-
if ( !ok && !srText.isEmpty() )
507-
{
508-
ok = crs.createFromUserInput( srText );
509-
}
510-
if ( !ok )
511-
crs = QgsCoordinateReferenceSystem::fromProj( result.PQgetvalue( 0, 3 ) );
512-
}
513-
sCrsCache.insert( srid, crs );
514-
}
515-
}
516-
}
459+
if ( conn ) crs = conn->sridToCrs( srid );
517460
return crs;
518461
}
519462

‎src/providers/postgres/qgspostgresprovider.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ class QgsPostgresProvider final: public QgsVectorDataProvider
4949
{
5050
Q_OBJECT
5151

52+
friend class TestQgsPostgresProvider;
53+
5254
public:
5355

5456
static const QString POSTGRES_KEY;

‎tests/src/providers/testqgspostgresprovider.cpp

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
* *
1414
***************************************************************************/
1515
#include "qgstest.h"
16+
#include "qgsconfig.h" // for ENABLE_PGTEST
1617
#include <QObject>
1718
#include <QRegularExpression>
1819
#include <QRegularExpressionMatch>
@@ -21,12 +22,43 @@
2122
#include <qgspostgresconn.h>
2223
#include <qgsfields.h>
2324

24-
2525
class TestQgsPostgresProvider: public QObject
2626
{
2727
Q_OBJECT
28+
29+
private:
30+
31+
#ifdef ENABLE_PGTEST
32+
QgsPostgresConn *_connection;
33+
34+
35+
QgsPostgresConn *getConnection()
36+
{
37+
if ( ! _connection )
38+
{
39+
const char *connstring = getenv( "QGIS_PGTEST_DB" );
40+
if ( !connstring ) connstring = "service=qgis_test";
41+
_connection = QgsPostgresConn::connectDb( connstring, true );
42+
}
43+
return _connection;
44+
}
45+
#endif
46+
2847
private slots:
2948

49+
void initTestCase() // will be called before the first testfunction is executed.
50+
{
51+
#ifdef ENABLE_PGTEST
52+
this->_connection = 0;
53+
#endif
54+
}
55+
void cleanupTestCase() // will be called after the last testfunction was executed.
56+
{
57+
#ifdef ENABLE_PGTEST
58+
if ( this->_connection ) this->_connection->unref();
59+
#endif
60+
}
61+
3062
void decodeHstore();
3163
void decodeHstoreNoQuote();
3264
void decodeArray2StringList();
@@ -41,6 +73,9 @@ class TestQgsPostgresProvider: public QObject
4173
void testDecodeDateTimes();
4274
void testQuotedValueBigInt();
4375
void testWhereClauseFids();
76+
#ifdef ENABLE_PGTEST
77+
void testEwktInOut();
78+
#endif
4479
};
4580

4681

@@ -428,5 +463,23 @@ void TestQgsPostgresProvider::testWhereClauseFids()
428463
<< "\"fld_int\"=43 AND \"fld\"::text='PostGIS too!'" );
429464
}
430465

466+
#ifdef ENABLE_PGTEST
467+
void TestQgsPostgresProvider::testEwktInOut()
468+
{
469+
QGSTEST_NEED_PGTEST_DB();
470+
471+
QgsPostgresConn *conn = getConnection();
472+
QVERIFY( conn != nullptr );
473+
QgsReferencedGeometry g;
474+
QString ewkt_obtained;
475+
476+
g = QgsPostgresProvider::fromEwkt( "SRID=4326;LINESTRING(0 0,-5 2)", conn );
477+
QVERIFY( ! g.isNull() );
478+
QCOMPARE( g.crs().authid(), "EPSG:4326" );
479+
ewkt_obtained = QgsPostgresProvider::toEwkt( g, conn );
480+
QCOMPARE( ewkt_obtained, "SRID=4326;LineString (0 0, -5 2)" );
481+
}
482+
#endif // ENABLE_PGTEST
483+
431484
QGSTEST_MAIN( TestQgsPostgresProvider )
432485
#include "testqgspostgresprovider.moc"

0 commit comments

Comments
 (0)
Please sign in to comment.