Skip to content

Commit

Permalink
Merge pull request #9879 from rouault/mitab_alterfielddefn
Browse files Browse the repository at this point in the history 
MITAB .tab: AlterFieldDefn(): fix data corruption when altering (for …
  • Loading branch information
@rouault
rouault committed May 10, 2024
2 parents cd4932b + d5cd568 commit a1a462e
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 33 deletions.
106 changes: 106 additions & 0 deletions autotest/ogr/ogr_mitab.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2812,3 +2812,109 @@ def test_ogr_mitab_label_without_text(tmp_vsimem):
def test_ogr_mitab_write_LCC_2SP_non_metre_unit(tmp_vsimem, ext):

_test_srs(tmp_vsimem, "EPSG:2277", ext=ext) # "NAD83 / Texas Central (ftUS)"


###############################################################################


def test_ogr_mitab_alter_field_defn_integer_width_4(tmp_vsimem):

filename = str(tmp_vsimem / "foo.tab")

ds = ogr.GetDriverByName("MapInfo File").CreateDataSource(filename)
lyr = ds.CreateLayer("foo")
fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger)
fld_defn.SetWidth(5)
lyr.CreateField(fld_defn)

# Changing field defn while no feature has been written is OK
idx = lyr.GetLayerDefn().GetFieldIndex("int_field")

new_fld_defn = ogr.FieldDefn("real_field", ogr.OFTReal)
new_fld_defn.SetWidth(15)
new_fld_defn.SetPrecision(6)
assert lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
assert fld_defn.GetType() == ogr.OFTReal
assert fld_defn.GetWidth() == 15
assert fld_defn.GetPrecision() == 6

new_fld_defn = ogr.FieldDefn("real_field", ogr.OFTReal)
new_fld_defn.SetWidth(30)
new_fld_defn.SetPrecision(6)
assert lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
assert fld_defn.GetType() == ogr.OFTReal
assert fld_defn.GetWidth() == 20
assert fld_defn.GetPrecision() == 6

new_fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger)
# Use 4 as this is also sizeof(int32) and there was a confusion before
# the fix linked with that test between decimal width and binary width.
new_fld_defn.SetWidth(4)
assert lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
assert fld_defn.GetType() == ogr.OFTInteger
assert fld_defn.GetWidth() == 4

f = ogr.Feature(lyr.GetLayerDefn())
f["int_field"] = 1234
lyr.CreateFeature(f)
f = None

new_fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger64)
with gdal.quiet_errors():
assert (
lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) != ogr.OGRERR_NONE
)
fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
assert fld_defn.GetType() == ogr.OFTInteger
assert fld_defn.GetWidth() == 4

ds = None

ds = ogr.Open(filename, update=1)
lyr = ds.GetLayer(0)
idx = lyr.GetLayerDefn().GetFieldIndex("int_field")
fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
assert fld_defn.GetWidth() == 4
# We don't change anything actually
assert lyr.AlterFieldDefn(idx, fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
assert fld_defn.GetType() == ogr.OFTInteger
assert fld_defn.GetWidth() == 4
f = lyr.GetNextFeature()
assert f["int_field"] == 1234


###############################################################################


def test_ogr_mitab_alter_field_defn_to_string(tmp_vsimem):

filename = str(tmp_vsimem / "foo.tab")

ds = ogr.GetDriverByName("MapInfo File").CreateDataSource(filename)
lyr = ds.CreateLayer("foo")
fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger)
fld_defn.SetWidth(5)
lyr.CreateField(fld_defn)
f = ogr.Feature(lyr.GetLayerDefn())
f["int_field"] = 1234
lyr.CreateFeature(f)
f = None
ds = None

ds = ogr.Open(filename, update=1)
lyr = ds.GetLayer(0)
idx = lyr.GetLayerDefn().GetFieldIndex("int_field")
new_fld_defn = ogr.FieldDefn("str_field", ogr.OFTString)
assert (
lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_NAME_FLAG | ogr.ALTER_TYPE_FLAG)
== ogr.OGRERR_NONE
)
fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
assert fld_defn.GetType() == ogr.OFTString
assert fld_defn.GetWidth() == 254
f = lyr.GetNextFeature()
assert f["str_field"] == "1234"
64 changes: 43 additions & 21 deletions ogr/ogrsf_frmts/mitab/mitab_datfile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,19 +748,20 @@ static int TABDATFileSetFieldDefinition(TABDATFieldDef *psFieldDef,
else if (nWidth == 0)
nWidth = 254; // char fields.

strncpy(psFieldDef->szName, pszName, sizeof(psFieldDef->szName) - 1);
psFieldDef->szName[sizeof(psFieldDef->szName) - 1] = '\0';
snprintf(psFieldDef->szName, sizeof(psFieldDef->szName), "%s", pszName);
psFieldDef->eTABType = eType;
psFieldDef->byLength = static_cast<GByte>(nWidth);
psFieldDef->byDecimals = static_cast<GByte>(nPrecision);
psFieldDef->byDecimals = 0;

switch (eType)
{
case TABFChar:
psFieldDef->cType = 'C';
psFieldDef->byLength = static_cast<GByte>(nWidth);
break;
case TABFDecimal:
psFieldDef->cType = 'N';
psFieldDef->byLength = static_cast<GByte>(nWidth);
psFieldDef->byDecimals = static_cast<GByte>(nPrecision);
break;
case TABFInteger:
psFieldDef->cType = 'C';
Expand Down Expand Up @@ -1236,8 +1237,8 @@ int TABDATFile::ReorderFields(int *panMap)
/* AlterFieldDefn() */
/************************************************************************/

int TABDATFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
int nFlags)
int TABDATFile::AlterFieldDefn(int iField, const OGRFieldDefn *poSrcFieldDefn,
OGRFieldDefn *poNewFieldDefn, int nFlags)
{
if (m_fp == nullptr)
{
Expand All @@ -1261,43 +1262,45 @@ int TABDATFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
}

TABFieldType eTABType = m_pasFieldDef[iField].eTABType;
int nWidth = m_pasFieldDef[iField].byLength;
int nPrecision = m_pasFieldDef[iField].byDecimals;
int nWidthDummy = 0;
int nPrecisionDummy = 0;
int nWidth = poSrcFieldDefn->GetWidth();
int nPrecision = poSrcFieldDefn->GetPrecision();
if (nFlags & ALTER_TYPE_FLAG)
{
if (IMapInfoFile::GetTABType(poNewFieldDefn, &eTABType, &nWidthDummy,
&nPrecisionDummy) < 0)
if (IMapInfoFile::GetTABType(poNewFieldDefn, &eTABType, nullptr,
nullptr) < 0)
return -1;
}
if (nFlags & ALTER_WIDTH_PRECISION_FLAG)
{
TABFieldType eTABTypeDummy;
if (IMapInfoFile::GetTABType(poNewFieldDefn, &eTABTypeDummy, &nWidth,
// Instead of taking directly poNewFieldDefn->GetWidth()/GetPrecision(),
// use GetTABType() to take into account .dat limitations on
// width & precision to clamp what user might have specify
if (IMapInfoFile::GetTABType(poNewFieldDefn, nullptr, &nWidth,
&nPrecision) < 0)
return -1;
}

if ((nFlags & ALTER_TYPE_FLAG) &&
eTABType != m_pasFieldDef[iField].eTABType)
{
if (eTABType != TABFChar)
if (eTABType != TABFChar && m_numRecords > 0)
{
CPLError(CE_Failure, CPLE_NotSupported,
"Can only convert to OFTString");
return -1;
}
if ((nFlags & ALTER_WIDTH_PRECISION_FLAG) == 0)
if (eTABType == TABFChar && (nFlags & ALTER_WIDTH_PRECISION_FLAG) == 0)
nWidth = 254;
}

if (nFlags & ALTER_WIDTH_PRECISION_FLAG)
{
if (eTABType != TABFChar && nWidth != m_pasFieldDef[iField].byLength)
if (eTABType != TABFChar && nWidth != poSrcFieldDefn->GetWidth() &&
m_numRecords > 0)
{
CPLError(CE_Failure, CPLE_NotSupported,
"Resizing only supported on String fields");
CPLError(
CE_Failure, CPLE_NotSupported,
"Resizing only supported on String fields on non-empty layer");
return -1;
}
}
Expand Down Expand Up @@ -1330,12 +1333,31 @@ int TABDATFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
}
if (nFlags & ALTER_WIDTH_PRECISION_FLAG)
{
m_pasFieldDef[iField].byLength = static_cast<GByte>(nWidth);
m_pasFieldDef[iField].byDecimals = static_cast<GByte>(nPrecision);
if (eTABType == TABFChar || eTABType == TABFDecimal)
m_pasFieldDef[iField].byLength = static_cast<GByte>(nWidth);
if (eTABType == TABFDecimal)
m_pasFieldDef[iField].byDecimals =
static_cast<GByte>(nPrecision);
}
return 0;
}

const bool bWidthPrecisionPreserved =
(nWidth == poSrcFieldDefn->GetWidth() &&
nPrecision == poSrcFieldDefn->GetPrecision());
if (eTABType == m_pasFieldDef[iField].eTABType && bWidthPrecisionPreserved)
{
return 0;
}

if (eTABType != TABFChar)
{
// should hopefully not happen given all above checks
CPLError(CE_Failure, CPLE_NotSupported,
"Unsupported AlterFieldDefn() operation");
return -1;
}

// Otherwise we need to do a temporary file.
TABDATFile oTempFile(GetEncoding());
CPLString osOriginalFile(m_pszFname);
Expand Down
12 changes: 8 additions & 4 deletions ogr/ogrsf_frmts/mitab/mitab_imapinfofile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,8 @@ int IMapInfoFile::GetTABType(const OGRFieldDefn *poField,
{
TABFieldType eTABType;
int nWidth = poField->GetWidth();
int nPrecision = poField->GetPrecision();
int nPrecision =
poField->GetType() == OFTReal ? poField->GetPrecision() : 0;

if (poField->GetType() == OFTInteger)
{
Expand Down Expand Up @@ -519,9 +520,12 @@ int IMapInfoFile::GetTABType(const OGRFieldDefn *poField,
return -1;
}

*peTABType = eTABType;
*pnWidth = nWidth;
*pnPrecision = nPrecision;
if (peTABType)
*peTABType = eTABType;
if (pnWidth)
*pnWidth = nWidth;
if (pnPrecision)
*pnPrecision = nPrecision;

return 0;
}
Expand Down
6 changes: 4 additions & 2 deletions ogr/ogrsf_frmts/mitab/mitab_priv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,8 @@ typedef struct TABDATFieldDef_t
{
char szName[11];
char cType;
GByte byLength;
GByte
byLength; /* caution: for a native .dat file, this is a binary width for most types */
GByte byDecimals;

TABFieldType eTABType;
Expand Down Expand Up @@ -1876,7 +1877,8 @@ class TABDATFile

int DeleteField(int iField);
int ReorderFields(int *panMap);
int AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn, int nFlags);
int AlterFieldDefn(int iField, const OGRFieldDefn *poSrcFieldDefn,
OGRFieldDefn *poNewFieldDefn, int nFlags);

int SyncToDisk();

Expand Down
23 changes: 17 additions & 6 deletions ogr/ogrsf_frmts/mitab/mitab_tabfile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2839,18 +2839,17 @@ OGRErr TABFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
return OGRERR_FAILURE;
}

if (m_poDATFile->AlterFieldDefn(iField, poNewFieldDefn, nFlagsIn) == 0)
OGRFieldDefn *poFieldDefn = m_poDefn->GetFieldDefn(iField);
if (m_poDATFile->AlterFieldDefn(iField, poFieldDefn, poNewFieldDefn,
nFlagsIn) == 0)
{
m_bNeedTABRewrite = TRUE;

OGRFieldDefn *poFieldDefn = m_poDefn->GetFieldDefn(iField);
auto oTemporaryUnsealer(poFieldDefn->GetTemporaryUnsealer());
if ((nFlagsIn & ALTER_TYPE_FLAG) &&
poNewFieldDefn->GetType() != poFieldDefn->GetType())
{
poFieldDefn->SetType(poNewFieldDefn->GetType());
if ((nFlagsIn & ALTER_WIDTH_PRECISION_FLAG) == 0)
poFieldDefn->SetWidth(254);
}
if (nFlagsIn & ALTER_NAME_FLAG)
{
Expand All @@ -2859,11 +2858,23 @@ OGRErr TABFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
m_oSetFields.insert(
CPLString(poNewFieldDefn->GetNameRef()).toupper());
}
if ((nFlagsIn & ALTER_WIDTH_PRECISION_FLAG) &&
poFieldDefn->GetType() == OFTString)
if (poFieldDefn->GetType() == OFTString)
{
poFieldDefn->SetWidth(m_poDATFile->GetFieldWidth(iField));
}
else if (nFlagsIn & ALTER_WIDTH_PRECISION_FLAG)
{
poFieldDefn->SetWidth(poNewFieldDefn->GetWidth());
poFieldDefn->SetPrecision(poNewFieldDefn->GetPrecision());
}

// Take into account .dat limitations on width & precision to clamp
// what user might have specify
int nWidth = 0;
int nPrecision = 0;
GetTABType(poFieldDefn, nullptr, &nWidth, &nPrecision);
poFieldDefn->SetWidth(nWidth);
poFieldDefn->SetPrecision(nPrecision);

if (m_eAccessMode == TABReadWrite)
WriteTABFile();
Expand Down

0 comments on commit a1a462e

Please sign in to comment.