From 2650f8327779c6d58baf1bf246b1d9f8bbbe9272 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Tue, 30 Jan 2024 14:48:31 +0100 Subject: [PATCH 1/2] Doc: OpenFileGDB: document OPENFILEGDB_DEFAULT_STRING_WIDTH configuration option (fixes #9165) --- doc/source/drivers/vector/openfilegdb.rst | 7 +++++++ ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h | 2 ++ ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp | 2 +- .../openfilegdb/ogropenfilegdblayer_write.cpp | 12 +++++++++--- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/doc/source/drivers/vector/openfilegdb.rst b/doc/source/drivers/vector/openfilegdb.rst index dfc67aa55e24..93e7f4505c7a 100644 --- a/doc/source/drivers/vector/openfilegdb.rst +++ b/doc/source/drivers/vector/openfilegdb.rst @@ -92,6 +92,13 @@ available: using the native spatial index. See `Spatial filtering`_. +- .. config:: OPENFILEGDB_DEFAULT_STRING_WIDTH + :choices: + + Width of string fields to use on creation, when the width specified to + CreateField() is the unspecified value 0. This defaults to 65536. + + Dataset open options -------------------- diff --git a/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h b/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h index 8c734761077a..f353f89a7320 100644 --- a/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h +++ b/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h @@ -48,6 +48,8 @@ std::string OFGDBGenerateUUID(); int OGROpenFileGDBIsComparisonOp(int op); +constexpr int OPENFILEGDB_DEFAULT_STRING_WIDTH = 65536; + // UUID of object type constexpr const char *pszFolderTypeUUID = "{f3783e6f-65ca-4514-8315-ce3985dad3b1}"; diff --git a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp index a559d0234e6d..62938657153f 100644 --- a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp +++ b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp @@ -664,7 +664,7 @@ int OGROpenFileGDBLayer::BuildLayerDefinition() // string width is 0, we pick up 65536 by default to mean unlimited // string length, but we do not want to advertise such a big number. if (eType == OFTString && - (nWidth < 65536 || + (nWidth < OPENFILEGDB_DEFAULT_STRING_WIDTH || CPLTestBool(CPLGetConfigOption( "OPENFILEGDB_REPORT_GENUINE_FIELD_WIDTH", "NO")))) { diff --git a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp index 66e988e6cd6b..068324140812 100644 --- a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp +++ b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp @@ -1352,9 +1352,15 @@ OGRErr OGROpenFileGDBLayer::CreateField(const OGRFieldDefn *poFieldIn, { // We can't use a 0 width value since that prevents ArcMap // from editing (#5952) - nWidth = atoi(CPLGetConfigOption("OPENFILEGDB_DEFAULT_STRING_WIDTH", - "65536")); - if (nWidth < 65536) + nWidth = OPENFILEGDB_DEFAULT_STRING_WIDTH; + if (const char *pszVal = CPLGetConfigOption( + "OPENFILEGDB_DEFAULT_STRING_WIDTH", nullptr)) + { + const int nVal = atoi(pszVal); + if (nVal >= 0) + nWidth = nVal; + } + if (nWidth < OPENFILEGDB_DEFAULT_STRING_WIDTH) poField->SetWidth(nWidth); } } From 6a9caf873ab87d7f6816125c5053decfad24a5f1 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 1 Feb 2024 11:53:33 +0100 Subject: [PATCH 2/2] [Lint] OpenFileGDB: rename constant to DEFAULT_STRING_WIDTH and add extensive comments --- ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h | 12 +++++++++++- .../openfilegdb/ogropenfilegdblayer.cpp | 7 ++++--- .../openfilegdb/ogropenfilegdblayer_write.cpp | 14 ++++++++++---- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h b/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h index f353f89a7320..d2b36d4e2b6b 100644 --- a/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h +++ b/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h @@ -48,7 +48,17 @@ std::string OFGDBGenerateUUID(); int OGROpenFileGDBIsComparisonOp(int op); -constexpr int OPENFILEGDB_DEFAULT_STRING_WIDTH = 65536; +// The FileGeodatabase format does not really allow strings of arbitrary width +// in the XML and .gdbtable header declaration. They must have a non-zero +// maximum width. But if we put it to a huge value (let's say 1 billion), this +// causes crashes in some Esri products (cf #5952, perhaps they allocate +// std::string's to that maximum size?). +// Hence this default of a relative large but not too large +// width when creating a OGR string field width of unspecified width. +// Note that when opening a FileGeodatabase with string fields of that width, +// we do not advertise it in OGRFieldDefn::GetWidth() but we advertise 0 instead, +// to allow round-tripping. +constexpr int DEFAULT_STRING_WIDTH = 65536; // UUID of object type constexpr const char *pszFolderTypeUUID = diff --git a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp index 62938657153f..8302cf69e249 100644 --- a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp +++ b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp @@ -661,10 +661,11 @@ int OGROpenFileGDBLayer::BuildLayerDefinition() oFieldDefn.SetAlternativeName(poGDBField->GetAlias().c_str()); oFieldDefn.SetSubType(eSubType); // On creation in the FileGDB driver (GDBFieldTypeToLengthInBytes) if - // string width is 0, we pick up 65536 by default to mean unlimited - // string length, but we do not want to advertise such a big number. + // string width is 0, we pick up DEFAULT_STRING_WIDTH=65536 by default + // to mean unlimited string length, but we do not want to advertise + // such a big number. if (eType == OFTString && - (nWidth < OPENFILEGDB_DEFAULT_STRING_WIDTH || + (nWidth < DEFAULT_STRING_WIDTH || CPLTestBool(CPLGetConfigOption( "OPENFILEGDB_REPORT_GENUINE_FIELD_WIDTH", "NO")))) { diff --git a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp index 068324140812..ec6429b9550d 100644 --- a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp +++ b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp @@ -1350,9 +1350,11 @@ OGRErr OGROpenFileGDBLayer::CreateField(const OGRFieldDefn *poFieldIn, nWidth = poField->GetWidth(); if (nWidth == 0) { - // We can't use a 0 width value since that prevents ArcMap - // from editing (#5952) - nWidth = OPENFILEGDB_DEFAULT_STRING_WIDTH; + // Hard-coded non-zero default string width if the user doesn't + // override it with the below configuration option. + // See comment at declaration of DEFAULT_STRING_WIDTH for more + // details + nWidth = DEFAULT_STRING_WIDTH; if (const char *pszVal = CPLGetConfigOption( "OPENFILEGDB_DEFAULT_STRING_WIDTH", nullptr)) { @@ -1360,7 +1362,11 @@ OGRErr OGROpenFileGDBLayer::CreateField(const OGRFieldDefn *poFieldIn, if (nVal >= 0) nWidth = nVal; } - if (nWidth < OPENFILEGDB_DEFAULT_STRING_WIDTH) + // Advertise a non-zero user-modified width back to the created + // OGRFieldDefn, only if it is less than the hard-coded default + // value (this will avoid potential issues with excessively large + // field width afterwards) + if (nWidth < DEFAULT_STRING_WIDTH) poField->SetWidth(nWidth); } }