Merge pull request #39043 from elpaso/bugfix-gh39036-39025-relations-…

…quoting PG fix relation discovery with table/schema that require quotes
qgis · Oct 1, 2020 · ebeb016 · ebeb016
2 parents 398131d + 52663c7
commit ebeb016
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 16 deletions.
diff --git a/scripts/spell_check/.agignore b/scripts/spell_check/.agignore
@@ -69,4 +69,5 @@ tests/testdata/layouts/sample_project.qgs
 tests/testdata/layouts/2x_template_attributetable.qpt
 tests/testdata/plugindependencies_data.json
 tests/testdata/projects/test-project-with-relations.qgs
+tests/testdata/provider/testdata_pg_relations.sql
 mac/development.md
diff --git a/src/providers/postgres/qgspostgresprovider.cpp b/src/providers/postgres/qgspostgresprovider.cpp
@@ -4901,6 +4901,7 @@ QList<QgsRelation> QgsPostgresProvider::discoverRelations( const QgsVectorLayer
     QgsLogger::warning( "Error getting the foreign keys of " + mTableName + ": invalid connection" );
     return result;
   }
+
   QString sql(
     "WITH foreign_keys AS "
     "  ( SELECT c.conname, "
@@ -4919,7 +4920,11 @@ QList<QgsRelation> QgsPostgresProvider::discoverRelations( const QgsVectorLayer
     "      WHERE oid = c.confrelid) as constraint_schema "
     "   FROM pg_constraint c "
     "   WHERE contype = 'f' "
-    "     AND c.conrelid::regclass = " + QgsPostgresConn::quotedValue( QString( mSchemaName + "." +  mTableName ) ) + "::regclass ) "
+    "     AND c.conrelid::regclass = " +
+    QgsPostgresConn::quotedValue( QgsPostgresConn::quotedIdentifier( mSchemaName ) +
+                                  '.' +
+                                  QgsPostgresConn::quotedIdentifier( mTableName ) ) +
+    "::regclass ) "
     "SELECT fk.conname as constraint_name, "
     "       a.attname as column_name, "
     "       fk.constraint_schema, "
@@ -4947,8 +4952,17 @@ QList<QgsRelation> QgsPostgresProvider::discoverRelations( const QgsVectorLayer
   {
     const QString name = sqlResult.PQgetvalue( row, 0 );
     const QString fkColumn = sqlResult.PQgetvalue( row, 1 );
-    const QString refSchema = sqlResult.PQgetvalue( row, 2 );
-    const QString refTable = sqlResult.PQgetvalue( row, 3 );
+    QString refSchema = sqlResult.PQgetvalue( row, 2 );
+    QString refTable = sqlResult.PQgetvalue( row, 3 );
+    // Strip quotes
+    if ( refTable.startsWith( '"' ) && refTable.endsWith( '"' ) )
+    {
+      refTable = refTable.mid( 1, refTable.length() - 2 );
+    }
+    if ( refSchema.startsWith( '"' ) && refSchema.endsWith( '"' ) )
+    {
+      refSchema = refSchema.mid( 1, refSchema.length() - 2 );
+    }
     const QString refColumn = sqlResult.PQgetvalue( row, 4 );
     const QString position = sqlResult.PQgetvalue( row, 5 );
     if ( ( position == QLatin1String( "1" ) ) || ( nbFound == 0 ) )

diff --git a/tests/src/python/test_qgsrelationpostgres.py b/tests/src/python/test_qgsrelationpostgres.py
@@ -31,33 +31,54 @@ class TestQgsRelationPostgresql(unittest.TestCase):
 
     @classmethod
     def setUpClass(cls):
-        """
-        Setup the involved layers and relations for a n:m relation
-        :return:
-        """
+
         cls.dbconn = 'service=\'qgis_test\''
         if 'QGIS_PGTEST_DB' in os.environ:
             cls.dbconn = os.environ['QGIS_PGTEST_DB']
-        # Create test layer
-        tables = ['c_amgmt_amgmt_lot', 'c_batiment_bat_lot', 'c_ens_immo_amgmt', 'c_ens_immo_bat', 'c_terrain_ens_immo', 't_actes', 't_adresse', 't_amgmt', 't_amgmt_lot', 't_bat', 't_bat_lot', 't_ens_immo', 't_terrain']
-        cls.vl_tables = ['vl_c_amgmt_amgmt_lot', 'vl_c_batiment_bat_lot', 'vl_c_ens_immo_amgmt', 'vl_c_ens_immo_bat', 'vl_c_terrain_ens_immo', 'vl_t_actes', 'vl_t_adresse', 'vl_t_amgmt', 'vl_t_amgmt_lot', 'vl_t_bat', 'vl_t_bat_lot', 'vl_t_ens_immo', 'vl_t_terrain']
-
-        for i in range(len(tables)):
-            cls.vl_tables[i] = QgsVectorLayer(cls.dbconn + ' sslmode=disable key=\'pk\' table="relations"."{}" sql='.format(tables[i]), tables[i], 'postgres')
-            assert(cls.vl_tables[i].isValid())
-            QgsProject.instance().addMapLayer(cls.vl_tables[i])
 
         cls.relMgr = QgsProject.instance().relationManager()
 
+    def setUp(self):
+        """
+        Setup the involved layers and relations for a n:m relation
+        :return:
+        """
+
+        QgsProject.instance().clear()
+
     def test_discover_relations(self):
         """
         Test the automatic discovery of relations
         """
-        relations = self.relMgr.discoverRelations([], self.vl_tables)
+
+        # Create test layer
+        tables = ['c_amgmt_amgmt_lot', 'c_batiment_bat_lot', 'c_ens_immo_amgmt', 'c_ens_immo_bat', 'c_terrain_ens_immo', 't_actes', 't_adresse', 't_amgmt', 't_amgmt_lot', 't_bat', 't_bat_lot', 't_ens_immo', 't_terrain']  # spellok
+        vl_tables = ['vl_c_amgmt_amgmt_lot', 'vl_c_batiment_bat_lot', 'vl_c_ens_immo_amgmt', 'vl_c_ens_immo_bat', 'vl_c_terrain_ens_immo', 'vl_t_actes', 'vl_t_adresse', 'vl_t_amgmt', 'vl_t_amgmt_lot', 'vl_t_bat', 'vl_t_bat_lot', 'vl_t_ens_immo', 'vl_t_terrain']  # spellok
+
+        for i in range(len(tables)):
+            vl_tables[i] = QgsVectorLayer(self.dbconn + ' sslmode=disable key=\'pk\' table="relations"."{}" sql='.format(tables[i]), tables[i], 'postgres')
+            assert(vl_tables[i].isValid())
+            QgsProject.instance().addMapLayer(vl_tables[i])
+
+        relations = self.relMgr.discoverRelations([], vl_tables)
         self.assertEqual(len(relations), 10)
         self.assertEqual(sum([len(r.referencingFields()) for r in relations]), 18)
         self.assertEqual(sum([len(r.referencedFields()) for r in relations]), 18)
 
+    def test_discover_relations_spaced(self):
+        """Test regression https://github.com/qgis/QGIS/issues/39025 and https://github.com/qgis/QGIS/issues/39036"""
+
+        vl_parent = QgsVectorLayer(self.dbconn + ' sslmode=disable key=\'pk\' table="spaced schema"."spaced parent" sql=', 'parent', 'postgres')
+        self.assertTrue(vl_parent.isValid())
+        vl_child = QgsVectorLayer(self.dbconn + ' sslmode=disable key=\'pk\' table="spaced schema"."spaced child" sql=', 'child', 'postgres')
+        self.assertTrue(vl_child.isValid())
+
+        QgsProject.instance().clear()
+        QgsProject.instance().addMapLayers([vl_child, vl_parent])
+
+        relations = self.relMgr.discoverRelations([], [vl_child, vl_parent])
+        self.assertEqual(len(relations), 1)
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/testdata/provider/testdata_pg_relations.sql b/tests/testdata/provider/testdata_pg_relations.sql
@@ -838,3 +838,128 @@ ALTER TABLE ONLY relations.t_terrain
 ALTER TABLE ONLY relations.t_terrain
     ADD CONSTRAINT t_terrain_fk1 FOREIGN KEY (fk_acte) REFERENCES relations.t_actes(cnumero);
 
+
+
+--
+-- Spaced schema and table
+--
+
+ALTER TABLE IF EXISTS ONLY "spaced schema"."spaced child" DROP CONSTRAINT IF EXISTS parent_fk;
+ALTER TABLE IF EXISTS ONLY "spaced schema"."spaced parent" DROP CONSTRAINT IF EXISTS "spaced parent_pkey";
+ALTER TABLE IF EXISTS ONLY "spaced schema"."spaced child" DROP CONSTRAINT IF EXISTS "spaced child_pkey";
+ALTER TABLE IF EXISTS "spaced schema"."spaced parent" ALTER COLUMN id DROP DEFAULT;
+ALTER TABLE IF EXISTS "spaced schema"."spaced child" ALTER COLUMN id DROP DEFAULT;
+DROP SEQUENCE IF EXISTS "spaced schema"."spaced parent_id_seq";
+DROP TABLE IF EXISTS "spaced schema"."spaced parent";
+DROP SEQUENCE IF EXISTS "spaced schema"."spaced child_id_seq";
+DROP TABLE IF EXISTS "spaced schema"."spaced child";
+DROP SCHEMA IF EXISTS "spaced schema";
+--
+-- Name: spaced schema; Type: SCHEMA; Schema: -; Owner: -
+--
+
+CREATE SCHEMA "spaced schema";
+
+
+SET default_tablespace = '';
+
+SET default_with_oids = false;
+
+--
+-- Name: spaced child; Type: TABLE; Schema: spaced schema; Owner: -
+--
+
+CREATE TABLE "spaced schema"."spaced child" (
+    id integer NOT NULL,
+    name character varying,
+    parent_id integer
+);
+
+
+--
+-- Name: spaced child_id_seq; Type: SEQUENCE; Schema: spaced schema; Owner: -
+--
+
+CREATE SEQUENCE "spaced schema"."spaced child_id_seq"
+    AS integer
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
+--
+-- Name: spaced child_id_seq; Type: SEQUENCE OWNED BY; Schema: spaced schema; Owner: -
+--
+
+ALTER SEQUENCE "spaced schema"."spaced child_id_seq" OWNED BY "spaced schema"."spaced child".id;
+
+
+--
+-- Name: spaced parent; Type: TABLE; Schema: spaced schema; Owner: -
+--
+
+CREATE TABLE "spaced schema"."spaced parent" (
+    id integer NOT NULL,
+    name character varying
+);
+
+
+--
+-- Name: spaced parent_id_seq; Type: SEQUENCE; Schema: spaced schema; Owner: -
+--
+
+CREATE SEQUENCE "spaced schema"."spaced parent_id_seq"
+    AS integer
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
+--
+-- Name: spaced parent_id_seq; Type: SEQUENCE OWNED BY; Schema: spaced schema; Owner: -
+--
+
+ALTER SEQUENCE "spaced schema"."spaced parent_id_seq" OWNED BY "spaced schema"."spaced parent".id;
+
+
+--
+-- Name: spaced child id; Type: DEFAULT; Schema: spaced schema; Owner: -
+--
+
+ALTER TABLE ONLY "spaced schema"."spaced child" ALTER COLUMN id SET DEFAULT nextval('"spaced schema"."spaced child_id_seq"'::regclass);
+
+
+--
+-- Name: spaced parent id; Type: DEFAULT; Schema: spaced schema; Owner: -
+--
+
+ALTER TABLE ONLY "spaced schema"."spaced parent" ALTER COLUMN id SET DEFAULT nextval('"spaced schema"."spaced parent_id_seq"'::regclass);
+
+
+--
+-- Name: spaced child spaced child_pkey; Type: CONSTRAINT; Schema: spaced schema; Owner: -
+--
+
+ALTER TABLE ONLY "spaced schema"."spaced child"
+    ADD CONSTRAINT "spaced child_pkey" PRIMARY KEY (id);
+
+
+--
+-- Name: spaced parent spaced parent_pkey; Type: CONSTRAINT; Schema: spaced schema; Owner: -
+--
+
+ALTER TABLE ONLY "spaced schema"."spaced parent"
+    ADD CONSTRAINT "spaced parent_pkey" PRIMARY KEY (id);
+
+
+--
+-- Name: spaced child parent_fk; Type: FK CONSTRAINT; Schema: spaced schema; Owner: -
+--
+
+ALTER TABLE ONLY "spaced schema"."spaced child"
+    ADD CONSTRAINT parent_fk FOREIGN KEY (parent_id) REFERENCES "spaced schema"."spaced parent"(id);
+