Skip to content

Commit

Permalink
[gpkg] Fix adding field comments after alternative name
Browse files Browse the repository at this point in the history 
Fix constraints on gpkg_data_columns are violated when AlterFieldDefn
is called in some circumstances, eg when a comment is added
after an alternative name has already been set on the field.
  • Loading branch information
@nyalldawson
nyalldawson committed Nov 16, 2023
1 parent 4e44aeb commit 39bde2a
Show file tree
Hide file tree
Showing 2 changed files with 133 additions and 67 deletions.
117 changes: 98 additions & 19 deletions autotest/ogr/ogr_gpkg.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3474,7 +3474,9 @@ def test_ogr_gpkg_36(tmp_vsimem, tmp_path):
dbname = tmp_vsimem / "ogr_gpkg_36.gpkg"
ds = gdaltest.gpkg_dr.CreateDataSource(dbname)
lyr = ds.CreateLayer("test", geom_type=ogr.wkbPolygon)
lyr.CreateField(ogr.FieldDefn("foo", ogr.OFTString))
field = ogr.FieldDefn("foo", ogr.OFTString)
field.SetAlternativeName("constraint")
lyr.CreateField(field)
lyr.CreateField(ogr.FieldDefn("baz", ogr.OFTString))
f = ogr.Feature(lyr.GetLayerDefn())
f.SetFID(10)
Expand All @@ -3483,19 +3485,6 @@ def test_ogr_gpkg_36(tmp_vsimem, tmp_path):
lyr.CreateFeature(f)
f = None

ds.ExecuteSQL(
"""CREATE TABLE gpkg_data_columns (
table_name TEXT NOT NULL,
column_name TEXT NOT NULL,
name TEXT,
title TEXT,
description TEXT,
mime_type TEXT,
constraint_name TEXT,
CONSTRAINT pk_gdc PRIMARY KEY (table_name, column_name),
CONSTRAINT gdc_tn UNIQUE (table_name, name)
)"""
)
ds.ExecuteSQL(
"""CREATE TABLE gpkg_data_column_constraints (
constraint_name TEXT NOT NULL,
Expand All @@ -3509,14 +3498,18 @@ def test_ogr_gpkg_36(tmp_vsimem, tmp_path):
CONSTRAINT gdcc_ntv UNIQUE (constraint_name,
constraint_type, value))"""
)
ds.ExecuteSQL(
"INSERT INTO gpkg_data_columns VALUES('test', 'foo', 'constraint', NULL, NULL, NULL, NULL)"
)
ds.ExecuteSQL(
"INSERT INTO gpkg_extensions VALUES('test', 'foo', 'extension_name', 'definition', 'read-write')"
)
ds.ExecuteSQL("CREATE INDEX my_idx ON test(foo)")

# gpkg_data_columns should have been created because of AlternativeName set on field
sql_lyr = ds.ExecuteSQL(
"SELECT * FROM gpkg_data_columns WHERE table_name = 'test' AND column_name = 'foo'"
)
assert sql_lyr.GetFeatureCount() == 1
ds.ReleaseResultSet(sql_lyr)

# Metadata
lyr.SetMetadataItem("FOO", "BAR")
ds.ExecuteSQL(
Expand Down Expand Up @@ -3563,8 +3556,15 @@ def test_ogr_gpkg_36(tmp_vsimem, tmp_path):

# Full table rewrite
new_field_defn.SetUnique(True)
new_field_defn.SetAlternativeName("alt name")
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0

sql_lyr = ds.ExecuteSQL(
"SELECT * FROM gpkg_data_columns WHERE table_name = 'test' AND column_name = 'bar'"
)
assert sql_lyr.GetFeatureCount() == 1
ds.ReleaseResultSet(sql_lyr)

# Violation of not-null constraint
new_field_defn = ogr.FieldDefn("baz", ogr.OFTString)
new_field_defn.SetNullable(False)
Expand All @@ -3582,18 +3582,47 @@ def test_ogr_gpkg_36(tmp_vsimem, tmp_path):
pytest.fail()
f = None

sql_lyr = ds.ExecuteSQL(
"SELECT * FROM gpkg_data_columns WHERE table_name = 'test' AND column_name = 'bar'"
)
assert sql_lyr.GetFeatureCount() == 1
ds.ReleaseResultSet(sql_lyr)

# Just change the name, and run it outside an existing transaction
lyr.StartTransaction()
new_field_defn = ogr.FieldDefn("baw2", ogr.OFTString)
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_NAME_FLAG) == 0
lyr.CommitTransaction()

# Just change the name, and run it under an existing transaction
sql_lyr = ds.ExecuteSQL(
"SELECT * FROM gpkg_data_columns WHERE table_name = 'test' AND column_name = 'baw2'"
)
assert sql_lyr.GetFeatureCount() == 1
ds.ReleaseResultSet(sql_lyr)

lyr.ResetReading()
f = lyr.GetNextFeature()
if (
f.GetFID() != 10
or f["baw2"] != 10.5
or f.GetGeometryRef().ExportToWkt() != "POLYGON ((0 0,0 1,1 1,0 0))"
):
f.DumpReadable()
pytest.fail()
f = None

# Change the name and type, comment and alternative name,
# and run it under an existing transaction
lyr.StartTransaction()
new_field_defn = ogr.FieldDefn("baw", ogr.OFTString)
new_field_defn.SetAlternativeName("baw alt")
new_field_defn.SetComment("baw comment")
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0
lyr.CommitTransaction()

assert lyr.GetLayerDefn().GetFieldDefn(0).GetAlternativeName() == "baw alt"
assert lyr.GetLayerDefn().GetFieldDefn(0).GetComment() == "baw comment"

lyr.ResetReading()
f = lyr.GetNextFeature()
if (
Expand Down Expand Up @@ -3664,6 +3693,56 @@ def test_ogr_gpkg_36(tmp_vsimem, tmp_path):
ds = None


###############################################################################
# Test AlterFieldDefn()


def test_ogr_gpkg_36_alter_comment_after_alternative_name(tmp_vsimem, tmp_path):

dbname = tmp_vsimem / "ogr_gpkg_36a.gpkg"
ds = gdaltest.gpkg_dr.CreateDataSource(dbname)
lyr = ds.CreateLayer("test", geom_type=ogr.wkbPolygon)
field = ogr.FieldDefn("foo", ogr.OFTString)
lyr.CreateField(field)

new_field_defn = lyr.GetLayerDefn().GetFieldDefn(0)
new_field_defn.SetAlternativeName("alt name")
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALTERNATIVE_NAME_FLAG) == 0

# gpkg_data_columns should have been created because of AlternativeName set on field
sql_lyr = ds.ExecuteSQL(
"SELECT * FROM gpkg_data_columns WHERE table_name = 'test' AND column_name = 'foo'"
)
assert sql_lyr.GetFeatureCount() == 1
ds.ReleaseResultSet(sql_lyr)

new_field_defn = lyr.GetLayerDefn().GetFieldDefn(0)
new_field_defn.SetComment("alt comment")
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_COMMENT_FLAG) == 0

sql_lyr = ds.ExecuteSQL(
"SELECT * FROM gpkg_data_columns WHERE table_name = 'test' AND column_name = 'foo'"
)
assert sql_lyr.GetFeatureCount() == 1
ds.ReleaseResultSet(sql_lyr)

assert lyr.GetLayerDefn().GetFieldDefn(0).GetAlternativeName() == "alt name"
assert lyr.GetLayerDefn().GetFieldDefn(0).GetComment() == "alt comment"

ds.ExecuteSQL("VACUUM")

ds = None

assert validate(dbname, tmpdir=tmp_path)

# Try on read-only dataset
ds = ogr.Open(dbname)
lyr = ds.GetLayer(0)

assert lyr.GetLayerDefn().GetFieldDefn(0).GetAlternativeName() == "alt name"
assert lyr.GetLayerDefn().GetFieldDefn(0).GetComment() == "alt comment"


###############################################################################
# Test ReorderFields()

Expand Down
83 changes: 35 additions & 48 deletions ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6811,96 +6811,83 @@ OGRErr OGRGeoPackageTableLayer::AlterFieldDefn(int iFieldToAlter,

if (eErr == OGRERR_NONE)
{
bool bRunDoSpecialProcessingForColumnCreation = false;
bool bDeleteFromGpkgDataColumns = false;
bool bNeedsEntryInGpkgDataColumns = false;

// field type
if (nActualFlags & ALTER_TYPE_FLAG)
{
if (poFieldDefnToAlter->GetSubType() == OFSTJSON &&
poNewFieldDefn->GetSubType() == OFSTNone)
{
bDeleteFromGpkgDataColumns = true;
}
else if (poFieldDefnToAlter->GetSubType() == OFSTNone &&
poNewFieldDefn->GetType() == OFTString &&
poNewFieldDefn->GetSubType() == OFSTJSON)
{
bRunDoSpecialProcessingForColumnCreation = true;
}

poFieldDefnToAlter->SetSubType(OFSTNone);
poFieldDefnToAlter->SetType(poNewFieldDefn->GetType());
poFieldDefnToAlter->SetSubType(poNewFieldDefn->GetSubType());
}
if (poFieldDefnToAlter->GetType() == OFTString &&
poFieldDefnToAlter->GetSubType() == OFSTJSON)
{
bNeedsEntryInGpkgDataColumns = true;
}

// name
if (nActualFlags & ALTER_NAME_FLAG)
{
poFieldDefnToAlter->SetName(poNewFieldDefn->GetNameRef());
}

// width/precision
if (nActualFlags & ALTER_WIDTH_PRECISION_FLAG)
{
poFieldDefnToAlter->SetWidth(poNewFieldDefn->GetWidth());
poFieldDefnToAlter->SetPrecision(
poNewFieldDefn->GetPrecision());
}

// constraints
if (nActualFlags & ALTER_NULLABLE_FLAG)
poFieldDefnToAlter->SetNullable(poNewFieldDefn->IsNullable());
if (nActualFlags & ALTER_DEFAULT_FLAG)
poFieldDefnToAlter->SetDefault(poNewFieldDefn->GetDefault());
if (nActualFlags & ALTER_UNIQUE_FLAG)
poFieldDefnToAlter->SetUnique(poNewFieldDefn->IsUnique());

// domain
if ((nActualFlags & ALTER_DOMAIN_FLAG) &&
poFieldDefnToAlter->GetDomainName() !=
poNewFieldDefn->GetDomainName())
{
if (!poFieldDefnToAlter->GetDomainName().empty())
{
bDeleteFromGpkgDataColumns = true;
}

if (!poNewFieldDefn->GetDomainName().empty())
{
bRunDoSpecialProcessingForColumnCreation = true;
}

poFieldDefnToAlter->SetDomainName(
poNewFieldDefn->GetDomainName());
}
if (!poFieldDefnToAlter->GetDomainName().empty())
{
bNeedsEntryInGpkgDataColumns = true;
}

// alternative name
if ((nActualFlags & ALTER_ALTERNATIVE_NAME_FLAG) &&
strcmp(poFieldDefnToAlter->GetAlternativeNameRef(),
poNewFieldDefn->GetAlternativeNameRef()) != 0)
{
if (!std::string(poFieldDefnToAlter->GetAlternativeNameRef())
.empty())
{
bDeleteFromGpkgDataColumns = true;
}

if (!std::string(poNewFieldDefn->GetAlternativeNameRef())
.empty())
{
bRunDoSpecialProcessingForColumnCreation = true;
}

poFieldDefnToAlter->SetAlternativeName(
poNewFieldDefn->GetAlternativeNameRef());
}
if (!std::string(poFieldDefnToAlter->GetAlternativeNameRef())
.empty())
{
bNeedsEntryInGpkgDataColumns = true;
}

// comment
if ((nActualFlags & ALTER_COMMENT_FLAG) &&
poFieldDefnToAlter->GetComment() !=
poNewFieldDefn->GetComment())
{
if (!poFieldDefnToAlter->GetComment().empty())
{
bDeleteFromGpkgDataColumns = true;
}

if (!poNewFieldDefn->GetComment().empty())
{
bRunDoSpecialProcessingForColumnCreation = true;
}

poFieldDefnToAlter->SetComment(poNewFieldDefn->GetComment());
}
if (!poFieldDefnToAlter->GetComment().empty())
{
bNeedsEntryInGpkgDataColumns = true;
}

if (bDeleteFromGpkgDataColumns)
if (m_poDS->HasDataColumnsTable())
{
char *pszSQL = sqlite3_mprintf(
"DELETE FROM gpkg_data_columns WHERE "
Expand All @@ -6911,7 +6898,7 @@ OGRErr OGRGeoPackageTableLayer::AlterFieldDefn(int iFieldToAlter,
sqlite3_free(pszSQL);
}

if (bRunDoSpecialProcessingForColumnCreation)
if (bNeedsEntryInGpkgDataColumns)
{
if (!DoSpecialProcessingForColumnCreation(poFieldDefnToAlter))
eErr = OGRERR_FAILURE;
Expand Down

0 comments on commit 39bde2a

Please sign in to comment.