From 288977e64d5a2bddb50e4ec846c93692666ac593 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 22 Feb 2024 15:14:41 +1000 Subject: [PATCH] [gpkg] Also read relationships defined using foreign key constraints When reading relationships, always read relationships defined using foreign key constraints regardless of whether or not the related tables extension is in use. The related table extension only permits definition of many-to-many relationships, so there's a strong case for supporting one-to-many relationships defined outside of this extension. In fact it's what's recommended upstream: https://github.com/opengeospatial/geopackage/issues/678#issuecomment-1954402113 --- autotest/ogr/ogr_gpkg.py | 27 +++++++++++++ .../gpkg/ogrgeopackagedatasource.cpp | 18 +++++++-- ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h | 3 +- .../sqlite/ogrsqlitedatasource.cpp | 39 ++++++++++++++----- 4 files changed, 73 insertions(+), 14 deletions(-) diff --git a/autotest/ogr/ogr_gpkg.py b/autotest/ogr/ogr_gpkg.py index f0e3ae75487b..29e5482be2d0 100755 --- a/autotest/ogr/ogr_gpkg.py +++ b/autotest/ogr/ogr_gpkg.py @@ -7086,6 +7086,33 @@ def test_ogr_gpkg_relations(tmp_vsimem, tmp_path): assert rel.GetRightMappingTableFields() == ["related_id"] assert rel.GetRelatedTableType() == "features" + # a one-to-many relationship defined using foreign key constraints + ds = gdal.OpenEx(filename, gdal.OF_VECTOR | gdal.OF_UPDATE) + ds.ExecuteSQL( + "CREATE TABLE test_relation_a(artistid INTEGER PRIMARY KEY, artistname TEXT)" + ) + ds.ExecuteSQL( + "CREATE TABLE test_relation_b(trackid INTEGER, trackname TEXT, trackartist INTEGER, FOREIGN KEY(trackartist) REFERENCES test_relation_a(artistid))" + ) + ds = None + + ds = gdal.OpenEx(filename, gdal.OF_VECTOR | gdal.OF_UPDATE) + assert ds.GetRelationshipNames() == [ + "custom_type", + "test_relation_a_test_relation_b", + ] + assert ds.GetRelationship("custom_type") is not None + rel = ds.GetRelationship("test_relation_a_test_relation_b") + assert rel is not None + assert rel.GetName() == "test_relation_a_test_relation_b" + assert rel.GetLeftTableName() == "test_relation_a" + assert rel.GetRightTableName() == "test_relation_b" + assert rel.GetCardinality() == gdal.GRC_ONE_TO_MANY + assert rel.GetType() == gdal.GRT_ASSOCIATION + assert rel.GetLeftTableFields() == ["artistid"] + assert rel.GetRightTableFields() == ["trackartist"] + assert rel.GetRelatedTableType() == "features" + ds = None diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp index 30271c1c3457..1dbbc1661e02 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp @@ -2018,14 +2018,24 @@ void GDALGeoPackageDataset::ClearCachedRelationships() void GDALGeoPackageDataset::LoadRelationships() const { + m_osMapRelationships.clear(); + + std::vector oExcludedTables; if (HasGpkgextRelationsTable()) { LoadRelationshipsUsingRelatedTablesExtension(); + + for (const auto &oRelationship : m_osMapRelationships) + { + oExcludedTables.emplace_back( + oRelationship.second->GetMappingTableName()); + } } - else - { - LoadRelationshipsFromForeignKeys(); - } + + // Also load relationships defined using foreign keys (i.e. one-to-many + // relationships). Here we must exclude any relationships defined from the + // related tables extension, we don't want them included twice. + LoadRelationshipsFromForeignKeys(oExcludedTables); m_bHasPopulatedRelationships = true; } diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h b/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h index 0be1906e8ccf..c0d6f6231d5e 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h @@ -216,7 +216,8 @@ class OGRSQLiteBaseDataSource CPL_NON_FINAL : public GDALPamDataset OGRErr PragmaCheck(const char *pszPragma, const char *pszExpected, int nRowsExpected); - void LoadRelationshipsFromForeignKeys() const; + void LoadRelationshipsFromForeignKeys( + const std::vector &excludedTables) const; bool IsSpatialiteLoaded(); static int MakeSpatialiteVersionNumber(int x, int y, int z) diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp index 65cd215ff3cb..82e0fa2510c7 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp @@ -284,7 +284,10 @@ std::vector OGRSQLiteDataSource::GetRelationshipNames( { if (!m_bHasPopulatedRelationships) - LoadRelationshipsFromForeignKeys(); + { + m_osMapRelationships.clear(); + LoadRelationshipsFromForeignKeys({}); + } std::vector oasNames; oasNames.reserve(m_osMapRelationships.size()); @@ -305,7 +308,10 @@ OGRSQLiteDataSource::GetRelationship(const std::string &name) const { if (!m_bHasPopulatedRelationships) - LoadRelationshipsFromForeignKeys(); + { + m_osMapRelationships.clear(); + LoadRelationshipsFromForeignKeys({}); + } auto it = m_osMapRelationships.find(name); if (it == m_osMapRelationships.end()) @@ -707,15 +713,13 @@ OGRErr OGRSQLiteBaseDataSource::PragmaCheck(const char *pszPragma, /* LoadRelationshipsFromForeignKeys() */ /************************************************************************/ -void OGRSQLiteBaseDataSource::LoadRelationshipsFromForeignKeys() const +void OGRSQLiteBaseDataSource::LoadRelationshipsFromForeignKeys( + const std::vector &excludedTables) const { - m_osMapRelationships.clear(); - if (hDB) { - auto oResult = SQLQuery( - hDB, + std::string osSQL = "SELECT m.name, p.id, p.seq, p.\"table\" AS base_table_name, " "p.\"from\", p.\"to\", " "p.on_delete FROM sqlite_master m " @@ -728,8 +732,25 @@ void OGRSQLiteBaseDataSource::LoadRelationshipsFromForeignKeys() const // Same with Spatialite system tables "AND base_table_name NOT IN ('geometry_columns', " "'spatial_ref_sys', 'views_geometry_columns', " - "'virts_geometry_columns') " - "ORDER BY m.name"); + "'virts_geometry_columns') "; + if (!excludedTables.empty()) + { + std::string oExcludedTablesList; + for (const auto &osExcludedTable : excludedTables) + { + oExcludedTablesList += !oExcludedTablesList.empty() ? "," : ""; + oExcludedTablesList += + sqlite3_mprintf("'%q'", osExcludedTable.c_str()); + } + + osSQL += "AND base_table_name NOT IN (" + oExcludedTablesList + + ")" + " AND m.name NOT IN (" + + oExcludedTablesList + ") "; + } + osSQL += "ORDER BY m.name"; + + auto oResult = SQLQuery(hDB, osSQL.c_str()); if (!oResult) {